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

[Target] Change no-trap-after-noreturn to trap-after-noreturn #67925

Closed
wants to merge 3 commits into from

Conversation

chsigg
Copy link
Contributor

@chsigg chsigg commented Oct 1, 2023

This was suggested in 5b7a7ec to avoid too many negates (e.g. !NoTrapAfterNoreturn).

@chsigg
Copy link
Contributor Author

chsigg commented Oct 1, 2023

The buildkite failure is unrelated to this PR. The culprit is commit 6403287.

Copy link
Contributor

@nikic nikic left a comment

Choose a reason for hiding this comment

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

Please add a patch description justifying this change. As-is, this seems like a pointless rename to me.

@Artem-B
Copy link
Member

Artem-B commented Oct 2, 2023

Please add a patch description justifying this change. As-is, this seems like a pointless rename to me.

I've suggested renaming it to improve readability. Double negations in a single word are already hard to read, and when it's used in a negation, !NoTrapAfterNoreturn looks outright odd to me.

We don't need these changes, but I think it's a nice cleanup.

@nikic
Copy link
Contributor

nikic commented Oct 2, 2023

NoTrapAfterNoreturn is a modifier on TrapUnreachable. You enable traps and then disable some of them again. If you formulate this positively, it looks like TrapAfterNoreturn=true should work independently of TrapUnreachable (insert traps after noreturn calls only), which is not how this actually works.

@Artem-B
Copy link
Member

Artem-B commented Oct 3, 2023

OK. If the naming was intentional, I'm fine keeping it as is.

@chsigg chsigg closed this Oct 3, 2023
@MaskRay
Copy link
Member

MaskRay commented Oct 3, 2023

OK. If the naming was intentional, I'm fine keeping it as is.

I think a variable with a positive meaning (TrapAfterNoreturn) is preferred. You could make the change non-functional.

It's just that how TrapAfterNoreturn is set to true and false should be thought carefully.

@chsigg
Copy link
Contributor Author

chsigg commented Oct 3, 2023

I had considered that, but didn't like the idea of variable names not matching the flag name. I also think Nikita's concerns would still apply, because it's common to configure this through TargetOptions, not through command line arguments.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants