-
-
Notifications
You must be signed in to change notification settings - Fork 781
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
Fix: warning format #1592
Fix: warning format #1592
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is looking good, and particularly the fixes and cleanup in the HostIO code. It'll be nice to eliminate some more UB and get things better sorted out.
There are some changes we'd like to see made in the hostio_reply() code though and a query over format string changes involving enums. Hopefully we can find a suitable solution with the latter though.
b70229f
to
59492bd
Compare
Reverted changes to enum type format specifiers, given the discussion in the review comments, for now let's reduce the scope of this PR and address the enum type issue separately A note about format specifiers was added to HACKING.md, note that it was quickly thrown together and might not be optimal, suggestions are welcome |
8bc3677
to
741ce51
Compare
741ce51
to
b1fc7fc
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @perigoso ! @dragonmux showed me this PR, good job! Just found some grammar and typo nitpicks to share with you.
e3feb5a
to
271d889
Compare
Co-authored-by: amyspark <amy@amyspark.me>
271d889
to
38789dd
Compare
Hey @amyspark, thank you for the review and suggestions! 😃 I made a small change to the way it's formatted after applying the the changes, since the "note" "directive" moved inline, the break in the middle felt out of place, so I removed it. I think I'm happy with this as is. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Going to poke @dragonmux to give it the final thumbs up!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. thank you Amy for the SPaG pass on the hacking guide change, and thank you Perigoso for putting together such a good PR that fixes so many subtle things.
Merging! The only change here that needs testing, we don't have the capability right now to test.. and that's the HostIO code. It looks correct though, so that's going to have to do with the community giving us feedback.
- `uint8_t`: `PRIx8` | ||
- `uint16_t`: `PRIx16` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We'd probably just use %x for these, given integer promotion, but you are correct that technically we should be using the PRI macros everywhere on all fixed-width integer types, especially with raw types in new code being forbidden because using the raw types is non-portable for width constraints.
That said, this isn't a blocker, so we'll merge with this in.
Detailed description
Fix some warnings for
-Wformat=2 -Wformat-overflow=2 -Wformat-signedness -Wformat-truncation
See #1590 for context
Your checklist for this pull request
make PROBE_HOST=native
)make PROBE_HOST=hosted
)Closing issues