-
Notifications
You must be signed in to change notification settings - Fork 14.3k
[clang] Remove separate evaluation step for static class member init. #142713
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
Merged
efriedma-quic
merged 4 commits into
llvm:main
from
efriedma-quic:static-member-init-eval
Jun 17, 2025
Merged
Changes from all commits
Commits
Show all changes
4 commits
Select commit
Hold shift + click to select a range
9bc16d5
[clang] Remove separate evaluation step for static class member init.
efriedma-quic 3ddce15
Fix formatting.
efriedma-quic 39e6548
Merge remote-tracking branch 'origin/main' into static-member-init-eval
efriedma-quic 8573eb1
Add a few more tests.
efriedma-quic File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
Based on the example here: https://eel.is/c++draft/expr.const#28.5
I don't think the division by zero diagnostic makes sense.
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 example is illegal for the same reason you can't write
int f(); struct X { static const int x = f(); };
. Normally static data members aren't allowed to have initializers at all if they're notinline
. But there's a very narrow carveout in [class.static.data]p4: if the type is aconst
non-volatile
integer type, it's allowed if the initializer is constant expression.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.
So if this is constant evaluated we have UB (divide by zero) so then it can't be constant evaluated and therefore
__builtin_is_constant_evaluated()
must be zero and therefore no divide by zero.Which is the same logic used in the example in [expr.const]p28.5
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.
@shafik I think you're right based on the current standard wording, but that this is a bug in the wording of the standard. The situation described in [expr.const]/28.5's example is a fallback from constant evaluation of the initializer to runtime evaluation, and that fallback shouldn't be possible for a static const data member. Will ask CWG.
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.
Note that the rules also say that
x
is not usable in constant expressions and has dynamic initialization in this case. This really looks like a wording oversight :)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.
Oh, I see what you mean. the variable doesn't have constant initialization, so it's not manifestly constant-evaluated... but it's still technically a constant expression. We could implement this by guarding the diagnostic with an isCXX11ConstantExpr check.
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.
That said, I don't want to think about how to generate an understandable diagnostic for
static const int x = 1/(1-__builtin_is_constant_evaluated()) + 1/__builtin_is_constant_evaluated();
.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.
I also think this outcome is pretty surprising: https://godbolt.org/z/cobb3Px5Y
The new behavior seems like what we ought to be doing, even if the old behavior is the one the rules appear to require. All other implementations seem to be checking whether the variable is constant-initialized, not whether its initializer is a constant expression.
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.
Aha, the resolution for CWG1721 (not yet resolved but in "review" state, which means CWG thinks the wording is ready for final review before resolution) changes the rule to check whether the variable is constant-initialized (thanks @t3nsor for pointing this out!). So this PR is implementing the proposed resolution of that issue.
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.
Okay, updated the commit message with a reference to CWG1721.