39128: pw_tokenizer format #39629
Open
39128: pw_tokenizer format #39629
Conversation
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
There was a problem hiding this comment.
Code Review
The code changes replace the %.*s format specifier with %s and the InlineString macro to prevent format string vulnerabilities. The review suggests adding comments to explain the purpose of the InlineString macro and the pragmas used to suppress compiler warnings.
andy31415
reviewed
Jun 20, 2025
c0683ec to
5f728d3
Compare
|
PR #39629: Size comparison from 852d123 to 6342dd0 Full report (82 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, cyw30739, efr32, esp32, linux, nrfconnect, nxp, psoc6, qpg, stm32, telink, tizen)
|
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request refactors logging calls across the codebase to work with pw_tokenizer by replacing %.*s format specifiers with SPAN_TO_TRUNCATED_CSTR. I've identified a missing header dependency in the new macro's definition that needs to be addressed, and provided suggestions for cleaning up a now-redundant include and for considering a larger buffer size for the new macro.
src/credentials/attestation_verifier/TestDACRevocationDelegateImpl.cpp
Outdated
Show resolved
Hide resolved
|
PR #39629: Size comparison from 852d123 to 4eb9f95 Full report (82 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, cyw30739, efr32, esp32, linux, nrfconnect, nxp, psoc6, qpg, stm32, telink, tizen)
|
4eb9f95 to
2d9d946
Compare
|
PR #39629: Size comparison from 9d7db20 to 2d9d946 Full report (82 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, cyw30739, efr32, esp32, linux, nrfconnect, nxp, psoc6, qpg, stm32, telink, tizen)
|
bzbarsky-apple
requested changes
Jun 24, 2025
src/credentials/attestation_verifier/TestDACRevocationDelegateImpl.cpp
Outdated
Show resolved
Hide resolved
|
/gemini review |
|
PR #39629: Size comparison from 9d7db20 to 8bb9f83 Full report (82 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, cyw30739, efr32, esp32, linux, nrfconnect, nxp, psoc6, qpg, stm32, telink, tizen)
|
5ce9e7e to
f5987cc
Compare
|
PR #39629: Size comparison from 60dc012 to f5987cc Full report (82 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, cyw30739, efr32, esp32, linux, nrfconnect, nxp, psoc6, qpg, stm32, telink, tizen)
|
heinsaar
approved these changes
Jul 4, 2025
andy31415
reviewed
Jul 10, 2025
src/lib/support/StringBuilder.h
Outdated
| StringBuilder() : StringBuilderBase(mBuffer, kSize) {} | ||
|
|
||
| /// Constructor for char * and length |
There was a problem hiding this comment.
These comments do not seem to help me read the code. Please remove them or change them to add things that are not obvious from the code.
andy31415
reviewed
Jul 10, 2025
src/lib/support/StringBuilder.h
Outdated
| /// a preallocated sized string builder | ||
| template <size_t kSize> | ||
| /// A preallocated sized string builder | ||
| /// According to the specification there are fields with size up to 256 |
There was a problem hiding this comment.
This needs some updates: string builder has no direct reference in the specification, so we should probably say that a very common use case is to wrap around field content for logging purposes.
This does feel awkward though ... I wish we had a separate method instead like:
StringBuilder<257> NullTerminated(CharSpan rawData) { // ...that seems to be more obvious and would not tie in a StringBuilder with the chip specification directly.
andy31415
reviewed
Jul 10, 2025
| ChipLogProgress(NetworkProvisioning, "ESP NetworkCommissioningDelegate: SSID: %.*s", static_cast<int>(networkId.size()), | ||
| networkId.data()); | ||
| ChipLogProgress(NetworkProvisioning, "ESP NetworkCommissioningDelegate: SSID: %s", | ||
| StringBuilder(Uint8::to_const_char(networkId.data()), networkId.size()).c_str()); |
There was a problem hiding this comment.
I would propose a separate function/method that handles uint8* and does some sane conversions somehow (i.e. does not assume these are just printable characters).
andy31415
reviewed
Jul 10, 2025
src/lib/support/StringBuilder.h
Outdated
| StringBuilder(const char * data, size_t length) : StringBuilder() { AddFormat("%.*s", static_cast<int>(length), data); } | ||
|
|
||
| /// Constructor for CharSpan | ||
| StringBuilder(const CharSpan & span) : StringBuilder(span.data(), span.size()) {} |
There was a problem hiding this comment.
Please make use of all new things you added in unit tests: https://github.com/project-chip/connectedhomeip/blob/master/src/lib/support/tests/TestStringBuilder.cpp exists and in this case we should have very good test coverage.
|
PR #39629: Size comparison from 1446be8 to 2a43382 Full report (83 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, cyw30739, efr32, esp32, linux, nrfconnect, nxp, psoc6, qpg, stm32, telink, tizen)
|
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.
Testing
This change is addressing issue #39128
This is because pw_tokenizer does not support format "%.*s" that we are using to format non null-terminated strings.