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

cleanup: Start of no-implicit-bool compliance; use prefix-increment. #75

Merged
merged 1 commit into from
May 3, 2024

Conversation

iphydf
Copy link
Contributor

@iphydf iphydf commented Apr 5, 2024

Semantics of postfix-increment are to return the value it had before the increment, encouraging code like f(..., b++), which is better written as f(..., b); ++b; to avoid doing too many possibly mutating operations in one statement/expression.

@camgunz
Copy link
Owner

camgunz commented Apr 14, 2024

I try to keep the C++-isms to a minimum in CMP and I'm not convinced of the benefits here. I did pull in the CI changes (thanks!), but I think unless there are errors in C++ compilers that can only be fixed by switching from idiomatic C to idiomatic C++ I don't think this is necessary. Could be convinced though!

Semantics of postfix-increment are to return the value it had before the
increment, encouraging code like `f(..., b++)`, which is better written
as `f(..., b); ++b;` to avoid doing too many possibly mutating
operations in one statement/expression.
@iphydf
Copy link
Contributor Author

iphydf commented May 1, 2024

Ok, I've reverted the CMP_NULL change for now, keeping the prefix-incr change and moving a local variable to a deeper scope (still C89 compatible).

@camgunz
Copy link
Owner

camgunz commented May 3, 2024

OK I can live with that, thanks for doing it :)

@camgunz camgunz merged commit 6b5f99d into camgunz:master May 3, 2024
1 check passed
@iphydf iphydf deleted the cpp branch May 3, 2024 13:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants