Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

print: Added size_t print format specifier #20194

Merged
merged 8 commits into from
Dec 21, 2023

Conversation

fzi-haxel
Copy link
Contributor

Contribution description

As discussed in #20182, this PR adds custom print format specifiers for size_t (e.g., PRIuSIZE) that doesn't assume sizeof(unsigned) == sizeof(size_t) to architecture.h and changes the size_t printing recommendation in CODING_CONVENTIONS.md

Furthermore, the bulk of this PR consists of changes that modify print statements with size_t arguments to use the new print format specifiers.

@maribu mentioned a possible switch to picolib in the near future. This would provide support for the currently problematic %z format specifier. If this is the preferred solution, I can easily replace the PRIuSIZE, PRIxSIZE, etc. macros with the corresponding %z modifiers and remove the custom print format specifiers.

Testing procedure

This shouldn't change any test or application.
To test the debug statement, set the appropriate ENABLE_DEBUG macro (or enable it globally).

Issues/PRs references

See also #20182, #20154

@github-actions github-actions bot added Area: network Area: Networking Area: doc Area: Documentation Area: tests Area: tests and testing framework Area: pkg Area: External package ports Area: drivers Area: Device drivers Area: OTA Area: Over-the-air updates Area: CoAP Area: Constrained Application Protocol implementations Area: USB Area: Universal Serial Bus Area: sys Area: System Area: examples Area: Example Applications labels Dec 19, 2023
@benpicco benpicco added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Dec 19, 2023
@riot-ci
Copy link

riot-ci commented Dec 19, 2023

Murdock results

✔️ PASSED

475a551 unittests/tests-fib: test cleanup

Success Failures Total Runtime
8098 0 8098 11m:15s

Artifacts

Copy link
Member

@maribu maribu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thx, this looks good to me. I have a number of nitpicks inline, though.

drivers/cc110x/cc110x_netdev.c Outdated Show resolved Hide resolved
examples/emcute_mqttsn/main.c Outdated Show resolved Hide resolved
sys/clif/clif.c Outdated Show resolved Hide resolved
sys/include/architecture.h Outdated Show resolved Hide resolved
sys/include/architecture.h Show resolved Hide resolved
tests/unittests/tests-fib/tests-fib.c Outdated Show resolved Hide resolved
tests/unittests/tests-fib/tests-fib.c Outdated Show resolved Hide resolved
tests/unittests/tests-fib/tests-fib.c Outdated Show resolved Hide resolved
tests/unittests/tests-fib/tests-fib.c Outdated Show resolved Hide resolved
tests/unittests/tests-fib/tests-fib.c Outdated Show resolved Hide resolved
@fzi-haxel
Copy link
Contributor Author

Thanks for the detailed review.

Copy link
Member

@OlegHahm OlegHahm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't look through everything and only found a few minor things/nitpicks.

drivers/cc2420/cc2420.c Outdated Show resolved Hide resolved
drivers/enc28j60/enc28j60.c Outdated Show resolved Hide resolved
sys/include/architecture.h Outdated Show resolved Hide resolved
@fzi-haxel
Copy link
Contributor Author

Should I squash the fixups and improve the commit messages a bit, or should I wait for more reviews?

@maribu
Copy link
Member

maribu commented Dec 21, 2023

Feel free to squash

fzi-haxel and others added 8 commits December 21, 2023 12:02
- Added `PRIuSIZE`, `PRIxSIZE`, etc. to `architecure.h`
- Changed `CODING_CONVENTIONS.md` regarding `size_t` printing

Co-authored-by: Marian Buschsieweke <marian.buschsieweke@posteo.net>
Co-authored-by: Marian Buschsieweke <marian.buschsieweke@posteo.net>
Co-authored-by: Marian Buschsieweke <marian.buschsieweke@posteo.net>
Co-authored-by: Marian Buschsieweke <marian.buschsieweke@posteo.net>
Co-authored-by: Marian Buschsieweke <marian.buschsieweke@posteo.net>
- Added helper function to avoid 'format-truncation' warning
- Changed all `size_t` types to `unsigned`
- Changed function names from `_FIB_` to `_fib_`
@fzi-haxel fzi-haxel force-pushed the pr/size_t_print_modifier branch from cdb9ba9 to 475a551 Compare December 21, 2023 11:08
@fzi-haxel
Copy link
Contributor Author

I squashed the fixups as well as the one review commit, and changed the commit messages a bit.
I also rebased to the new master (which I now realize is a bit annoying in compare view - sorry for that), the code should be the same.

@maribu maribu added Type: cleanup The issue proposes a clean-up / The PR cleans-up parts of the codebase / documentation Process: API change Integration Process: PR contains or issue proposes an API change. Should be handled with care. labels Dec 21, 2023
@maribu maribu added this pull request to the merge queue Dec 21, 2023
Merged via the queue into RIOT-OS:master with commit 418bc2f Dec 21, 2023
28 checks passed
@fzi-haxel fzi-haxel deleted the pr/size_t_print_modifier branch January 8, 2024 13:05
fzi-haxel added a commit to fzi-haxel/RIOT that referenced this pull request Jan 12, 2024
@MrKevinWeiss MrKevinWeiss added this to the Release 2024.01 milestone Feb 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: CoAP Area: Constrained Application Protocol implementations Area: doc Area: Documentation Area: drivers Area: Device drivers Area: examples Area: Example Applications Area: network Area: Networking Area: OTA Area: Over-the-air updates Area: pkg Area: External package ports Area: sys Area: System Area: tests Area: tests and testing framework Area: USB Area: Universal Serial Bus CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Process: API change Integration Process: PR contains or issue proposes an API change. Should be handled with care. Type: cleanup The issue proposes a clean-up / The PR cleans-up parts of the codebase / documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants