Conversation
dragonmux
left a comment
There was a problem hiding this comment.
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
amyspark
left a comment
There was a problem hiding this comment.
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. |
amyspark
left a comment
There was a problem hiding this comment.
Going to poke @dragonmux to give it the final thumbs up!
There was a problem hiding this comment.
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.
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-truncationSee #1590 for context
Your checklist for this pull request
make PROBE_HOST=native)make PROBE_HOST=hosted)Closing issues