Clean some follow-up issues#43103
Merged
Merged
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Code Review
This pull request contains some follow-up cleanups. The removal of unnecessary comments in the test file and the added check in QNameString::EndsWith are good improvements.
However, I have concerns about the removal of the CHIP_DEVICE_CONFIG_ENABLE_THREAD_MESHCOP guards. The function declaration in ThreadRendezvousAnnouncement.h is now unguarded, while its definition in ThreadRendezvousAnnouncement.cpp remains guarded. This inconsistency can lead to linker errors if the feature is disabled. I've left comments suggesting to restore these guards for safety. If this feature is now mandatory, then the guards should be removed from both the header and the implementation file.
I am having trouble creating individual review comments. Click here to see my feedback.
src/app/server/ThreadRendezvousAnnouncement.cpp (26-30)
This guard around the includes seems useful and should probably be kept. These headers are only used by BuildThreadRendezvousAnnouncement, which is conditionally compiled based on CHIP_DEVICE_CONFIG_ENABLE_THREAD_MESHCOP. Keeping the guard avoids including unused headers when this feature is disabled.
|
PR #43103: Size comparison from 485755f to 73d02c2 Full report (11 builds for cc13x4_26x4, cc32xx, qpg, realtek, stm32)
|
bukepo
force-pushed
the
mcot-minor-fixes
branch
2 times, most recently
from
12275fd to
e48df20
Compare
|
PR #43103: Size comparison from 485755f to e48df20 Full report (35 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, efr32, esp32, nrfconnect, nxp, psoc6, qpg, realtek, stm32, telink)
|
Contributor
There was a problem hiding this comment.
Pull request overview
Follow-up cleanup for the Thread MeshCoP commissioning work (#42992), including build-guarding Thread-specific sources/tests and tightening QNameString::EndsWith() behavior when the string representation was truncated.
Changes:
- Make
QNameString::EndsWith()returnfalsewhen the underlying string builder did not fully fit. - Remove/comment/guard cleanup for Thread rendezvous announcement code and its unit test.
- Gate Thread rendezvous announcement sources (server + tests) behind
chip_device_config_enable_thread_meshcopin GN.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| src/lib/dnssd/minimal_mdns/core/QNameString.h | Adds a Fit() check before suffix comparison. |
| src/app/server/tests/TestThreadRendezvousAnnouncement.cpp | Removes an “expected entries” comment block in the TXT builder test. |
| src/app/server/tests/BUILD.gn | Attempts to conditionally include the Thread rendezvous unit test. |
| src/app/server/ThreadRendezvousAnnouncement.h | Removes #if CHIP_DEVICE_CONFIG_ENABLE_THREAD_MESHCOP header guard around declarations. |
| src/app/server/ThreadRendezvousAnnouncement.cpp | Makes minimal_mdns includes unconditional (file now built conditionally via GN). |
| src/app/server/BUILD.gn | Moves Thread rendezvous announcement sources/deps under chip_device_config_enable_thread_meshcop. |
andy31415
reviewed
Feb 12, 2026
andy31415
reviewed
Feb 12, 2026
bukepo
force-pushed
the
mcot-minor-fixes
branch
from
e48df20 to
284fb84
Compare
|
PR #43103: Size comparison from 385acf5 to 284fb84 Full report (3 builds for cc32xx, stm32)
|
bukepo
force-pushed
the
mcot-minor-fixes
branch
from
284fb84 to
970b9be
Compare
|
PR #43103: Size comparison from 385acf5 to 970b9be Full report (34 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, efr32, esp32, nrfconnect, nxp, psoc6, qpg, realtek, telink)
|
bukepo
force-pushed
the
mcot-minor-fixes
branch
from
970b9be to
2da1b73
Compare
|
PR #43103: Size comparison from 385acf5 to 2da1b73 Full report (12 builds for cc13x4_26x4, cc32xx, nrfconnect, qpg, realtek, stm32)
|
bukepo
force-pushed
the
mcot-minor-fixes
branch
from
2da1b73 to
f6eaf76
Compare
bukepo
force-pushed
the
mcot-minor-fixes
branch
from
f6eaf76 to
32fa514
Compare
|
PR #43103: Size comparison from 385acf5 to 32fa514 Full report (35 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, efr32, esp32, nrfconnect, nxp, psoc6, qpg, realtek, stm32, telink)
|
bukepo
force-pushed
the
mcot-minor-fixes
branch
from
32fa514 to
fe038ac
Compare
|
PR #43103: Size comparison from 977df9d to fe038ac Full report (35 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, efr32, esp32, nrfconnect, nxp, psoc6, qpg, realtek, stm32, telink)
|
andy31415
approved these changes
Feb 13, 2026
andy31415
added
the
sdk-maintainer-approved
Contributor
|
Thank you for the followup! |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
This commit is a follow-up of #42992 fixing remaining issues:
src/app/server/tests/TestThreadRendezvousAnnouncement.cppRelated issues
#42992
Testing
No new test added. This commit fixes style issues and strengthen the check of
QNameString::EndsWith()