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.

Sign up for GitHub

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

Jump to bottom

Use stdbool rather than custom booleans #821

Merged
merged 1 commit into from
Jan 15, 2025
Merged

Use stdbool rather than custom booleans #821

merged 1 commit into from
Jan 15, 2025

Conversation

saghul
Copy link
Contributor

@saghul saghul commented Jan 12, 2025 •
edited
Loading

Fixes: #817

@saghul saghul force-pushed the stdbool branch 12 times, most recently from ef5b966 to ffc3161 Compare January 13, 2025 09:44
@saghul saghul requested a review from bnoordhuis January 13, 2025 10:47
@saghul
Copy link
Contributor Author

saghul commented Jan 13, 2025

@bnoordhuis Can you pl take a quick peek? This became larger than I thought, but it did show that we were being a bit inconsistent with the boolean vs "boolean but -1 for exception" case. I feel this makes things more obvious.

@saghul saghul marked this pull request as ready for review January 13, 2025 14:32
bnoordhuis
bnoordhuis reviewed Jan 13, 2025
View reviewed changes
Copy link
Contributor

@bnoordhuis bnoordhuis left a comment

Choose a reason for hiding this comment

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

No real comments, looks fine to me. Was there anything in particular I should pay attention to?

@saghul
Copy link
Contributor Author

saghul commented Jan 14, 2025

I'd like an extra pair of eyes on the strict layout changes, but I need to have another look so I'll @ you again :-)

@saghul saghul force-pushed the stdbool branch 2 times, most recently from 93400f1 to ffc3161 Compare January 14, 2025 10:22
@saghul
Copy link
Contributor Author

saghul commented Jan 14, 2025

@bnoordhuis This is now ready for review. In the end there are no struct layout changes. Replacing the uint8_t is_foo : 1; occurrences was A Bad Idea (TM) because those booleans would end up taking a byte rather than a bit and everything becomes larger.

@bnoordhuis bnoordhuis mentioned this pull request Jan 14, 2025
Merged
bnoordhuis
bnoordhuis approved these changes Jan 14, 2025
View reviewed changes
Copy link
Contributor

@bnoordhuis bnoordhuis left a comment

Choose a reason for hiding this comment

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

RSLGTM

@saghul saghul force-pushed the stdbool branch 3 times, most recently from 250521a to df06917 Compare January 15, 2025 08:29
@saghul saghul merged commit a7914d1 into master Jan 15, 2025
59 checks passed
@saghul saghul deleted the stdbool branch January 15, 2025 13:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bnoordhuis bnoordhuis bnoordhuis approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Use stdbool?
2 participants
@saghul @bnoordhuis