-
Notifications
You must be signed in to change notification settings - Fork 15.2k
C++26 Annotation #166287
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?
C++26 Annotation #166287
Conversation
Recognize annotation starting token in attr parsing Signed-off-by: acassagnes <acassagnes@bloomberg.net>
Signed-off-by: acassagnes <acassagnes@bloomberg.net>
Add parsing for annotation Signed-off-by: acassagnes <acassagnes@bloomberg.net>
Signed-off-by: acassagnes <acassagnes@bloomberg.net>
Signed-off-by: acassagnes <acassagnes@bloomberg.net>
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
Signed-off-by: acassagnes <acassagnes@bloomberg.net>
|
@katzdm FYI 🙇 |
Signed-off-by: acassagnes <acassagnes@bloomberg.net> Undo dumb edit Signed-off-by: acassagnes <acassagnes@bloomberg.net>
Signed-off-by: acassagnes <acassagnes@bloomberg.net>
|
CC @erichkeane |
|
AST representation for struct [[=1, =2]] Foo4 {};is |
Add smoke test Signed-off-by: acassagnes <acassagnes@bloomberg.net>
clang/include/clang/Basic/Attr.td
Outdated
| code AdditionalMembers = [{}]; | ||
| // Any additional text that should be included verbatim after instantiating | ||
| // an attribute on a template. | ||
| code PostInstantiationStmts =[{}]; |
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 seems... particularly odd? Can you better explain what is going on here? That comment doesn't help very much. Also, needs a space after the =.
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.
Still needs attention.
clang/include/clang/Basic/Attr.td
Outdated
| SourceLocation EqLoc; | ||
|
|
||
| public: | ||
| static CXX26AnnotationAttr *Create(ASTContext &Ctx, \ |
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.
These creates don't seem right, they should be auto-generated. Or at least help suppress the ones that are already created?
The fact that this is putting so much extra data into an attribute is concerning.
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.
Those were not strictly needed, cleaned up.
clang/include/clang/Basic/Attr.td
Outdated
| }]; | ||
|
|
||
| let PostInstantiationStmts = [{ | ||
| Expr::EvalResult V; |
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.
Can you better explain why this is happening?
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 believe there might be a more canonical way to achieve that via Sema::InstantiateAttrs , I'm giving this a try and that will remove this PostInstantiateStmts thingy
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.
Gave it a try in 9ddf0a8
I assume (?) there are cases that will break and when I find those, we'll either fix it and keep this approach or revert to the more handcrafty PostInstantiationStmts
| "default scope specifier for attributes is incompatible with C++ standards " | ||
| "before C++17">, InGroup<CXXPre17Compat>, DefaultIgnore; | ||
| def warn_cxx26_compat_annotation : Warning< | ||
| "annotation is a C++26 extension">, InGroup<CXXPre26Compat>, DefaultIgnore; |
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.
| "annotation is a C++26 extension">, InGroup<CXXPre26Compat>, DefaultIgnore; | |
| "annotations are a C++26 extension">, InGroup<CXXPre26Compat>, DefaultIgnore; |
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.
Fixed
|
|
||
| static void handleCxx26AnnotationAttr(Sema &S, Decl *D, const ParsedAttr &AL) { | ||
| Expr *CE = AL.getArgAsExpr(0); | ||
| if (CE->isLValue()) { |
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.
Can you include the standardeeze here as to what exactly is happening/the rules of what should happen on the RHS here?
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.
Still would like this here, other comments not attended to in this func.
|
|
||
| CE = CopyResult.get(); | ||
| } else { | ||
| ExprResult RVExprResult = S.DefaultLvalueConversion(AL.getArgAsExpr(0)); |
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.
Is there a reason the normal copy-init like above is incorrect here?
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.
For this and the comment above, a sample like the following would not be properly handled by default lvalue conversion path
struct U {
bool V;
constexpr U(bool v) : V(v) {}
U(const U&) = delete; // #del-U
};
constexpr U u(true);
struct [[ =u ]] h2{}; // expected-error {{call to deleted constructor of 'U'}}
// expected-note@#del-U {{'U' has been explicitly marked deleted here}}My understanding is that for record type we basically do not do much besides returning the expression itself in DefaultLvalueConversion
clang/lib/Parse/ParseDeclCXX.cpp
Outdated
| // - We must not mix with an attribute | ||
| if (Tok.is(tok::equal)) { | ||
| if (!getLangOpts().CPlusPlus26) { | ||
| Diag(Tok.getLocation(), diag::warn_cxx26_compat_annotation); |
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 seems like it should be an error to me. Allowing annotations, even if ignored on earlier code, seems like a mistake.
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.
Made this an error by default
clang/lib/Parse/ParseDeclCXX.cpp
Outdated
| return; | ||
|
|
||
| IdentifierTable &IT = Actions.PP.getIdentifierTable(); | ||
| IdentifierInfo &Placeholder = IT.get("__annotation_placeholder"); |
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.
Hmm, this is a little strange unfortunately. Can we try to modify ParsedAttr to accept one without a name? We already have special cases in there for other attribute kinds, and annotations not having one seems sensible.
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 think it's doable, but I wonder if adding and handling that corner case is worth the wasted identifier ? Unless there is a more technical argument that I am missing
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.
Its more than just the identifier, its that this is too much of a 'hack' here. ParsedAttr/etc are designed to be extended for things like this (See the large number of ctors/addNews anyway).
The 'addNew' being used here is for generic attributes, and annotations are special enough to be worth extra effort.
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.
Thanks, I took a crack at it in 9109557 , is that what you had in mind ?
|
I think we should have a minimum of reflection (parsing, infra for meta functions) before considering merging that change . Note that P3795R0 - approved by the C++ committee last week allow attributes on function parameters |
cor3ntin
left a comment
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.
just making sure we don't merge that that before #164692
|
Thanks @erichkeane @cor3ntin And I am ok with Corentin comment, we can keep this open until we get enough reflection merged, so we can onboard relevant metafunctions here as well. |
Eh, I disagree. Parsing + AST tests (to make sure they do what they're supposed to!) is a beneficial small-bite of this feature that I think is worth merging on its own. Enabling these ONLY under the -freflection flag though would be a good idea. I think the meta-function querying them is a useful component to make sure these are fully usable, but IMO, there is enough functionality here (modulo a few small things) that we could merge this and simplify our review life from now on. |
Improve documentation Emit error if annotation syntax found in tablegen Signed-off-by: acassagnes <acassagnes@bloomberg.net>
Signed-off-by: acassagnes <acassagnes@bloomberg.net>
|
I have addressed the quickest comments and the mishandled case on mixing attribute and annotation. |
tbaederr
left a comment
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.
You need to end your comments in a period (I'm just saying this before Aaron does).
| @@ -0,0 +1,35 @@ | |||
| // RUN: %clang_cc1 -std=c++26 -x c++ %s -verify | |||
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.
Does this also work when passing -fexperimental-new-constant-interpreter?
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 think so unless I passed it wrong ?
clang/include/clang/Basic/Attr.td
Outdated
|
|
||
| public: | ||
|
|
||
| APValue getValue() const { return Value; } |
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 think we should return a const reference here so we don't copy the APValue.
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.
done
| S.Diag(CE->getBeginLoc(), diag::err_attribute_argument_type) | ||
| << "C++26 annotation" << 5 << CE->getSourceRange(); | ||
| return; | ||
| } |
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.
Does the order make sense here? The comment says "evaluate to", but the check has nothing to do the the evaluated value, just the ConstantExpr type.
| S.Diag(P.first, P.second); | ||
|
|
||
| return; | ||
| } |
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'm a bit confused by the usage of ConstantExpr here, we evaluate it, so it presumably doesn't have an APValue set yet. But we also don't set the value on it here. Is that just missing or am I misunderstanding something?
Signed-off-by: acassagnes <acassagnes@bloomberg.net>
erichkeane
left a comment
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.
A release note might be a good idea here to show progress on reflection.
I think annotations should ALSO only work under the freflection flag for the short-term, but that isn't in place yet, so we might need to wait on that to merge this.
clang/include/clang/Basic/Attr.td
Outdated
| code AdditionalMembers = [{}]; | ||
| // Any additional text that should be included verbatim after instantiating | ||
| // an attribute on a template. | ||
| code PostInstantiationStmts =[{}]; |
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.
Still needs attention.
clang/lib/Parse/ParseDeclCXX.cpp
Outdated
| return; | ||
|
|
||
| IdentifierTable &IT = Actions.PP.getIdentifierTable(); | ||
| IdentifierInfo &Placeholder = IT.get("__annotation_placeholder"); |
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.
Its more than just the identifier, its that this is too much of a 'hack' here. ParsedAttr/etc are designed to be extended for things like this (See the large number of ctors/addNews anyway).
The 'addNew' being used here is for generic attributes, and annotations are special enough to be worth extra effort.
|
|
||
| static void handleCxx26AnnotationAttr(Sema &S, Decl *D, const ParsedAttr &AL) { | ||
| Expr *CE = AL.getArgAsExpr(0); | ||
| if (CE->isLValue()) { |
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.
Still would like this here, other comments not attended to in this func.
clang/lib/Sema/SemaDeclAttr.cpp
Outdated
|
|
||
| if (!CE->EvaluateAsConstantExpr(Result, S.Context, CEKind)) { | ||
| S.Diag(CE->getBeginLoc(), diag::err_attribute_argument_type) | ||
| << "C++26 annotation" << 4 << CE->getSourceRange(); |
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 don't see it addressed? Usually we'd do something like:
| << "C++26 annotation" << 4 << CE->getSourceRange(); | |
| << "C++26 annotation" << /*template arg=*/4 << CE->getSourceRange(); |
Though better (and preferred) would be to convert the diagnostic to be a enum_select and use the actual names for these uses.
Signed-off-by: acassagnes <acassagnes@bloomberg.net>
Signed-off-by: acassagnes <acassagnes@bloomberg.net>
🐧 Linux x64 Test Results
|
Signed-off-by: acassagnes <acassagnes@bloomberg.net>
Signed-off-by: acassagnes <acassagnes@bloomberg.net>
Signed-off-by: acassagnes <acassagnes@bloomberg.net>
Signed-off-by: acassagnes <acassagnes@bloomberg.net>
The ongoing work to support reflection done for ex. in #164692 is early yet, so the current PR does not add metafunctions that relate to annotations, since the eval mechanism isnt yet known.
The design is taken from the clang P2996 experimental compiler
WIP
TODO
🙇 Credits shall go to @katzdm, blame to @zebullax