steffenlarsen marked 2 inline comments as done.
steffenlarsen added inline comments.
Comment at: clang/lib/Parse/ParseDecl.cpp:497
+// Pack expansion is only allowed in the variadic argument at the end.
+const size_t attrNumArgs = attributeNumberOfArguments(*AttrN
steffenlarsen updated this revision to Diff 403187.
steffenlarsen marked an inline comment as done.
steffenlarsen added a comment.
Removing top-level `const` and adjusting style.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D114439/new/
https://reviews.llvm.org/D114439
Files:
clang/i
aaron.ballman added inline comments.
Comment at: clang/lib/Parse/ParseDecl.cpp:497
+// Pack expansion is only allowed in the variadic argument at the end.
+const size_t attrNumArgs = attributeNumberOfArguments(*AttrName);
+if (ArgExprs.size() + I + 1 < att
steffenlarsen marked an inline comment as done.
steffenlarsen added inline comments.
Comment at: clang/lib/Parse/ParseDecl.cpp:468
+
+ // General case. Parse all available expressions.
+ CommaLocsTy CommaLocs;
aaron.ballman wrote:
> I think it'd be good
steffenlarsen updated this revision to Diff 401910.
steffenlarsen added a comment.
Moved comment.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D114439/new/
https://reviews.llvm.org/D114439
Files:
clang/include/clang/Basic/Attr.td
clang/include/clang/Basic/DiagnosticParseKinds.td
aaron.ballman added inline comments.
Comment at: clang/lib/Parse/ParseDecl.cpp:468
+
+ // General case. Parse all available expressions.
+ CommaLocsTy CommaLocs;
I think it'd be good to move this comment up closer to the `else` on line 461
so that it's
steffenlarsen updated this revision to Diff 401580.
steffenlarsen added a comment.
Applied suggestions.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D114439/new/
https://reviews.llvm.org/D114439
Files:
clang/include/clang/Basic/Attr.td
clang/include/clang/Basic/DiagnosticParseKinds
steffenlarsen added a comment.
Thank you, @aaron.ballman ! I will update the revision in a moment with some of
the suggested changes.
Comment at: clang/lib/Parse/ParseDecl.cpp:436-438
+// Interpret "kw_this" as an identifier if the attributed requests it.
+if (
aaron.ballman added a comment.
Thanks for this, I think it's shaping up well!
Comment at: clang/include/clang/Basic/DiagnosticParseKinds.td:723
+def err_attribute_argument_parm_pack_not_supported : Error<
+ "attribute %0 does not support pack expansion in arguments">;
+def err
steffenlarsen updated this revision to Diff 399334.
steffenlarsen added a comment.
I have made the changes suggested in my previous comment. This makes
significantly more changes to the parsing of attribute arguments as the old
path was needed for attributes that allow both expressions and types
steffenlarsen added a comment.
I agree that reusing existing parser logic for this would be preferable, but as
@aaron.ballman mentions `Parser::ParseSimpleExpressionList` does not give us
the parameter pack expansion parsing we need. We could potentially unify it
with the logic from
https://gi
aaron.ballman added inline comments.
Comment at: clang/lib/Parse/ParseDecl.cpp:447
Actions.CorrectDelayedTyposInExpr(ParseAssignmentExpression()));
+
+if (Tok.is(tok::ellipsis)) {
erichkeane wrote:
> steffenlarsen wrote:
> > erichkeane wrote:
erichkeane added inline comments.
Comment at: clang/lib/Parse/ParseDecl.cpp:447
Actions.CorrectDelayedTyposInExpr(ParseAssignmentExpression()));
+
+if (Tok.is(tok::ellipsis)) {
steffenlarsen wrote:
> erichkeane wrote:
> > steffenlarsen wrote:
steffenlarsen added inline comments.
Comment at: clang/lib/Parse/ParseDecl.cpp:447
Actions.CorrectDelayedTyposInExpr(ParseAssignmentExpression()));
+
+if (Tok.is(tok::ellipsis)) {
erichkeane wrote:
> steffenlarsen wrote:
> > erichkeane wrote:
erichkeane added inline comments.
Comment at: clang/lib/Parse/ParseDecl.cpp:447
Actions.CorrectDelayedTyposInExpr(ParseAssignmentExpression()));
+
+if (Tok.is(tok::ellipsis)) {
steffenlarsen wrote:
> erichkeane wrote:
> > steffenlarsen wrote:
steffenlarsen added inline comments.
Comment at: clang/lib/Parse/ParseDecl.cpp:447
Actions.CorrectDelayedTyposInExpr(ParseAssignmentExpression()));
+
+if (Tok.is(tok::ellipsis)) {
erichkeane wrote:
> steffenlarsen wrote:
> > erichkeane wrote:
erichkeane added inline comments.
Comment at: clang/lib/Parse/ParseDecl.cpp:447
Actions.CorrectDelayedTyposInExpr(ParseAssignmentExpression()));
+
+if (Tok.is(tok::ellipsis)) {
steffenlarsen wrote:
> erichkeane wrote:
> > So I was thinking ab
steffenlarsen added inline comments.
Comment at: clang/lib/Parse/ParseDecl.cpp:447
Actions.CorrectDelayedTyposInExpr(ParseAssignmentExpression()));
+
+if (Tok.is(tok::ellipsis)) {
erichkeane wrote:
> So I was thinking about this overnight...
erichkeane added inline comments.
Comment at: clang/lib/Parse/ParseDecl.cpp:447
Actions.CorrectDelayedTyposInExpr(ParseAssignmentExpression()));
+
+if (Tok.is(tok::ellipsis)) {
So I was thinking about this overnight... I wonder if this logic
steffenlarsen added inline comments.
Comment at: clang/test/Parser/cxx0x-attributes.cpp:268
+ void faz [[clang::annotate("B", (Is + ...))]] (); // expected-warning {{pack
fold expression is a C++17 extension}}
+ void foz [[clang::annotate("C", Is...)]] ();
}
steffenlarsen updated this revision to Diff 392340.
steffenlarsen added a comment.
The new revision does the following:
1. Revisits the diagnostics. Now parameter packs will cause the same error on
attributes that do not support parameter packs in arguments, while attributes
that support it wil
steffenlarsen added inline comments.
Comment at: clang/include/clang/Basic/DiagnosticParseKinds.td:724
+ "attribute %0 does not support pack expansion in the last argument">;
+def err_attribute_parm_pack_last_argument_only : Error<
+ "pack expansion in attributes is restricted
erichkeane added inline comments.
Comment at: clang/include/clang/Basic/DiagnosticParseKinds.td:724
+ "attribute %0 does not support pack expansion in the last argument">;
+def err_attribute_parm_pack_last_argument_only : Error<
+ "pack expansion in attributes is restricted to
steffenlarsen added a comment.
Thank you both for the reviews! Consensus seems to be having support for pack
expansion at argument level for now and let more complicated logic be added
when there is an actual need. I have applied these changes as I understood them
and have added/adjusted the re
steffenlarsen updated this revision to Diff 391588.
steffenlarsen edited the summary of this revision.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D114439/new/
https://reviews.llvm.org/D114439
Files:
clang/include/clang/Basic/Attr.td
clang/include/clang/Basic/DiagnosticParseKinds.td
erichkeane added inline comments.
Comment at: clang/include/clang/Basic/Attr.td:544-546
+ // Set to true for attributes that support parameter pack expansion in its
+ // arguments.
+ bit ParmExpansionArgsSupport = 0;
aaron.ballman wrote:
> steffenlarsen wrote:
aaron.ballman added inline comments.
Comment at: clang/include/clang/Basic/Attr.td:544-546
+ // Set to true for attributes that support parameter pack expansion in its
+ // arguments.
+ bit ParmExpansionArgsSupport = 0;
steffenlarsen wrote:
> aaron.ballman wro
steffenlarsen added inline comments.
Comment at: clang/lib/Parse/ParseDecl.cpp:447
+ }
+ ArgExpr = Actions.ActOnPackExpansion(ArgExpr.get(), ConsumeToken());
+}
steffenlarsen wrote:
> erichkeane wrote:
> > Do you have a test for somethin
steffenlarsen added inline comments.
Comment at: clang/include/clang/Basic/Attr.td:544-546
+ // Set to true for attributes that support parameter pack expansion in its
+ // arguments.
+ bit ParmExpansionArgsSupport = 0;
aaron.ballman wrote:
> From a design per
erichkeane added a comment.
>> I think the safest bet is to be conservative and not allow mixing packs with
>> variadics, and not allowing multiple packs. We should be able to diagnose
>> that situation from ClangAttrEmitter.cpp to help attribute authors out.
>> However, it'd be worth adding a
aaron.ballman requested changes to this revision.
aaron.ballman added inline comments.
This revision now requires changes to proceed.
Comment at: clang/include/clang/Basic/Attr.td:544-546
+ // Set to true for attributes that support parameter pack expansion in its
+ // argument
erichkeane added a comment.
Aaron is way more familiar with this code than I am, but I've got some
suggestions for more tests in the parsing, we need to make sure that we handle
pack expansion completely here.
Comment at: clang/lib/Parse/ParseDecl.cpp:447
+ }
+
Tyker added a comment.
This seems like a good change to me. but i don't think my approval is enough
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D114439/new/
https://reviews.llvm.org/D114439
___
cfe-comm
steffenlarsen created this revision.
steffenlarsen added reviewers: erichkeane, aaron.ballman, Tyker, Naghasan.
Herald added a subscriber: jdoerfert.
steffenlarsen requested review of this revision.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.
These changes make the Clang
34 matches
Mail list logo
