-
Notifications
You must be signed in to change notification settings - Fork 15.3k
[clang]: reflection operator parsing for primitive types #164692
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
base: main
Are you sure you want to change the base?
Changes from 94 commits
cda974f
daf965a
5cfcf49
59c609c
c59b1d3
a9df861
9b353b7
88d8af8
245f466
482e25e
c2d6d7e
7c7ac79
61c7f94
78fe7c4
5ae0873
4503b11
4852569
d3f0f70
e93fa6e
e845cb8
2d8c1cb
7a331be
340d7c0
7c30c5d
08cd161
258283e
6b8aaf5
8660059
e6fec7b
95ec04c
8be44be
5af4ae2
13cbe6f
8bd9dc3
dacc226
442feeb
ad7a5e6
632c4d8
ae941f2
745cc22
f61f630
455549f
a7281de
72c191d
af47462
21b9784
0de3284
634c3dd
dc5b7cc
5a4efbd
ceafe4b
0ac0294
09dd041
b811940
7335550
ac23c88
bf7e78a
f1c0088
41c3a59
c213826
d0d0ab7
74150d7
cde51d9
9854d9e
d9d32ed
e8275a7
a22d917
3b0782f
fe481ca
29820dc
58ac339
8c104c4
56997b7
b38fb97
348348e
c78e314
a623415
f852125
e3c76a4
8a1852c
7450614
8a0f646
aa29b6c
f135dcc
08962b5
03b9653
f8d2d8a
336cb0d
acbc699
99950d7
96280f7
db1ad53
7fabcaa
bbe8da3
2d65e6a
f88ba6e
fadde00
b444df9
544dc12
a5ad36c
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -1851,6 +1851,11 @@ def err_placeholder_expected_auto_or_decltype_auto : Error< | |
| "expected 'auto' or 'decltype(auto)' after concept name">; | ||
| } | ||
|
|
||
| let CategoryName = "Reflection Diagnostics" in { | ||
| def err_cannot_reflect_operand : Error< | ||
| "expected reflectable entity">; | ||
|
||
| } | ||
|
|
||
| def warn_max_tokens : Warning< | ||
| "the number of preprocessor source tokens (%0) exceeds this token limit (%1)">, | ||
| InGroup<MaxTokens>, DefaultIgnore; | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -500,6 +500,7 @@ LANGOPT(BoundsSafety, 1, 0, NotCompatible, "Bounds safety extension for C") | |
| LANGOPT(EnableLifetimeSafety, 1, 0, NotCompatible, "Experimental lifetime safety analysis for C++") | ||
|
|
||
| LANGOPT(PreserveVec3Type, 1, 0, NotCompatible, "Preserve 3-component vector type") | ||
| LANGOPT(Reflection , 1, 0, NotCompatible, "C++26 Reflection") | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't think we want this as an option. IMO, we should just have this be turned on for C++26, and not enable-able for other language versions.
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I (at least partially) disagree. Reflection is so experimental I think it should be an option, but one that can only be enabled in C++26 mode. Given that the feature is changing in the last few minutes of getting C++26 out the door, I think we should treat reflection like we would a TS to some degree so users can opt out of it, especially because of the potential to break code (there's still parsing ambiguities with |
||
|
|
||
| #undef LANGOPT | ||
| #undef ENUM_LANGOPT | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -3694,6 +3694,11 @@ defm application_extension : BoolFOption<"application-extension", | |
| PosFlag<SetTrue, [], [ClangOption, CC1Option], | ||
| "Restrict code to those available for App Extensions">, | ||
| NegFlag<SetFalse>>; | ||
| defm reflection : BoolFOption<"reflection", | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Again, we should decide which versions we want this enabled for, but I don't think just enabling on a command line arg is going to be our direction forward.
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is it intentional that there are no driver changes in this PR to accept this option and consume it for the front-end?
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think this makes sense as a CC1-only option rather than a driver option. aka. the driver can say "C++26 mode? enable reflection by default", but there's still a hidden escape hatch to say
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I have no strong opinion on whether Reflection should (in the medium term) be opt-in or opt-out in C++26 mode. In the short term, it should be opt-in (and opting in via a CC1-only option is fine). In other words, (for now) we can make it a CC1-only option that, if enabled outside of C++26, generates an error.
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah, I agree completely, the default-off (opt-in) and only in C++26 mode are important to me. I think exposing this in the driver is likely premature.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. so right now its an cc1-only option, off by default, I will need to add c++26-only mode right?
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's not exactly just a As for making it C++26-only, you may be able to add a check in Clang to generate an error if the option ends up being on without C++26 being on as well.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @hubert-reinterpretcast I need to remove
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Yes.
I'm not familiar with that construct. Sorry. |
||
| LangOpts<"Reflection">, DefaultFalse, | ||
| PosFlag<SetTrue, [], [CC1Option], | ||
| "Enable C++26 reflection">, | ||
| NegFlag<SetFalse>>, ShouldParseIf<cplusplus.KeyPath>; | ||
| defm sized_deallocation : BoolFOption<"sized-deallocation", | ||
| LangOpts<"SizedDeallocation">, Default<cpp14.KeyPath>, | ||
| PosFlag<SetTrue, [], [], "Enable C++14 sized global deallocation functions">, | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -4946,6 +4946,12 @@ void CXXNameMangler::mangleExpression(const Expr *E, unsigned Arity, | |
| E = cast<ConstantExpr>(E)->getSubExpr(); | ||
| goto recurse; | ||
|
|
||
| case Expr::CXXReflectExprClass: { | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Will we need a similar case for the Microsoft mangler?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @katzdm can you help me on this question?
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @changkhothuychung Yes, absolutely; analogous changes should be made to Some context: A few things to note here:
@AaronBallman - Does that all sound right to you?
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
That all sounds correct to me!
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @AaronBallman after looking more into this, I think we can start working on this in subsequent PRs when more facilities are available. For now I'm trying to keep the scope of this PR small to get it merged. Does that sound good to you?
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ooof, doing this as a 'fallthrough' is probably a mistake. We should do this as a We should just error-out, like we do in the
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should we make
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We should probably just not do a fallthrough, and just copy/paste the stuff inside the !NullOpt into it. |
||
| // TODO(Reflection): implement this after introducing std::meta::info | ||
| // and add info in APValue | ||
| [[fallthrough]]; | ||
| } | ||
|
|
||
| // FIXME: invent manglings for all these. | ||
| case Expr::BlockExprClass: | ||
| case Expr::ChooseExprClass: | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -333,6 +333,28 @@ Parser::ParseRHSOfBinaryExpression(ExprResult LHS, prec::Level MinPrec) { | |
| Token OpToken = Tok; | ||
| ConsumeToken(); | ||
|
|
||
| // The reflection operator is not valid here (i.e., in the place of the | ||
| // operator token in a binary expression), so if reflection and blocks are | ||
| // enabled, we split caretcaret into two carets: the first being the binary | ||
| // operator and the second being the introducer for the block. | ||
| if (OpToken.is(tok::caretcaret)) { | ||
| assert(getLangOpts().Reflection && | ||
| "reflection support disabled - compile with -freflection"); | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. messages to users in 'asserts' is weird. If we want to say something like this, we should be diagnosing.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't think this should be a diagnostic - this is more of a "sanity check", since In light of that, maybe change the assert-message. Something like:
Or just omit the message.
|
||
| if (getLangOpts().Blocks) { | ||
| OpToken.setKind(tok::caret); | ||
| Token Caret; | ||
| { | ||
| Caret.startToken(); | ||
| Caret.setKind(tok::caret); | ||
| Caret.setLocation(Tok.getLocation()); | ||
| Caret.setLength(1); | ||
| } | ||
| UnconsumeToken(OpToken); | ||
| PP.EnterToken(Caret, /*IsReinject=*/true); | ||
| return ParseRHSOfBinaryExpression(LHS, MinPrec); | ||
| } | ||
| } | ||
|
|
||
| // If we're potentially in a template-id, we may now be able to determine | ||
| // whether we're actually in one or not. | ||
| if (OpToken.isOneOf(tok::comma, tok::greater, tok::greatergreater, | ||
|
|
@@ -1208,6 +1230,18 @@ Parser::ParseCastExpression(CastParseKind ParseKind, bool isAddressOfOperand, | |
| AllowSuffix = false; | ||
| Res = ParseUnaryExprOrTypeTraitExpression(); | ||
| break; | ||
| case tok::caretcaret: { | ||
| if (!getLangOpts().Reflection) { | ||
| NotCastExpr = true; | ||
| return ExprError(); | ||
| } | ||
|
|
||
| if (NotPrimaryExpression) | ||
| *NotPrimaryExpression = true; | ||
| AllowSuffix = false; | ||
| Res = ParseCXXReflectExpression(); | ||
| break; | ||
| } | ||
| case tok::ampamp: { // unary-expression: '&&' identifier | ||
| if (NotPrimaryExpression) | ||
| *NotPrimaryExpression = true; | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.