[PATCH] D114439: [Annotation] Allow parameter pack expansions in annotate attribute
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(*AttrName);
+if (ArgExprs.size() + I + 1 < attrNumArgs) {
aaron.ballman wrote:
> aaron.ballman wrote:
> > Coding style nits.
> We don't typically use top-level `const`, so that should go as well.
Completely missed that. My bad!
Comment at: clang/lib/Parse/ParseExpr.cpp:3374-3375
+if (EarlyTypoCorrection)
+ Expr = Actions.CorrectDelayedTyposInExpr(Expr);
+
aaron.ballman wrote:
> steffenlarsen wrote:
> > aaron.ballman wrote:
> > > steffenlarsen wrote:
> > > > aaron.ballman wrote:
> > > > > Not that I think this is a bad change, but it seems unrelated to the
> > > > > patch. Can you explain why you made the change?
> > > > Admittedly I am not sure why it is needed, but in the previous version
> > > > of the parsing for attribute parameters it does the typo-correction
> > > > immediately after parsing the expression and if this isn't added a
> > > > handful of tests fail.
> > > Ah, thanks for the info! So this is basically doing the same work as
> > > what's done on line 3410 in the late failure case -- I wonder if this
> > > should actually be tied to the `FailImmediatelyOnInvalidExpr` parameter
> > > instead of having its own parameter?
> > They could be unified in the current version, but I don't think there is an
> > actual relation between them.
> Do we fail tests if this isn't conditionalized at all (we always do the early
> typo correction)? (I'd like to avoid adding the new parameter because I'm not
> certain how a caller would determine whether they do or do not want that
> behavior. If we need the parameter, then so be it, but it seems like this is
> something generally useful.)
11 tests fail if we don't make it conditional. The most telling one is probably
`SemaCXX/typo-correction-delayed.cpp` which will have more errors if we do typo
correction early, so it seems that the delay is intentional.
Comment at: clang/test/Parser/cxx0x-attributes.cpp:262
-template void variadic() {
+template void variadic() {
void bar [[noreturn...]] (); // expected-error {{attribute 'noreturn' cannot
be used as an attribute pack}}
aaron.ballman wrote:
> You can back out the whitespace changes for an ever-so-slightly smaller
> patch. :-D
Absolutely. Clang-format was very insistent, but since it isn't added by this
patch I shouldn't worry.
Comment at: clang/test/Parser/cxx0x-attributes.cpp:276-277
+ void foz [[clang::annotate("E", 1, 2, 3, Is...)]] ();
+ void fiz [[clang::annotate("F", Is..., 1, 2, 3)]] ();
+ void fir [[clang::annotate("G", 1, Is..., 2, 3)]] ();
+}
aaron.ballman wrote:
> Shouldn't these cases also give the `// expected-error {{pack expansion in
> 'annotate' may only be applied to the last argument}}` diagnostic?
These should preferably pass. They are part of the "last" argument as
`annotate` has one non-variadic argument and one variadic argument. I agree
though that the error message is a little vague. Originally the error did
specify that it had to be "argument %1 or later" to try and convey this intent,
but maybe you have an idea for a clearer error message?
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D114439/new/
https://reviews.llvm.org/D114439
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D114439: [Annotation] Allow parameter pack expansions in annotate attribute
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/include/clang/Basic/Attr.td
clang/include/clang/Basic/DiagnosticParseKinds.td
clang/include/clang/Parse/Parser.h
clang/lib/Parse/ParseDecl.cpp
clang/lib/Parse/ParseExpr.cpp
clang/lib/Sema/SemaTemplateInstantiateDecl.cpp
clang/test/Parser/cxx0x-attributes.cpp
clang/test/SemaTemplate/attributes.cpp
clang/utils/TableGen/ClangAttrEmitter.cpp
Index: clang/utils/TableGen/ClangAttrEmitter.cpp
===
--- clang/utils/TableGen/ClangAttrEmitter.cpp
+++ clang/utils/TableGen/ClangAttrEmitter.cpp
@@ -1164,10 +1164,12 @@
};
class VariadicExprArgument : public VariadicArgument {
+bool AllowPack = false;
+
public:
VariadicExprArgument(const Record , StringRef Attr)
- : VariadicArgument(Arg, Attr, "Expr *")
-{}
+: VariadicArgument(Arg, Attr, "Expr *"),
+ AllowPack(Arg.getValueAsBit("AllowPack")) {}
void writeASTVisitorTraversal(raw_ostream ) const override {
OS << " {\n";
@@ -2264,6 +2266,47 @@
OS << "#endif // CLANG_ATTR_THIS_ISA_IDENTIFIER_ARG_LIST\n\n";
}
+static void emitClangAttrNumArgs(RecordKeeper , raw_ostream ) {
+ OS << "#if defined(CLANG_ATTR_NUM_ARGS)\n";
+ std::vector Attrs = Records.getAllDerivedDefinitions("Attr");
+ for (const auto *A : Attrs) {
+std::vector Args = A->getValueAsListOfDefs("Args");
+forEachUniqueSpelling(*A, [&](const FlattenedSpelling ) {
+ OS << ".Case(\"" << S.name() << "\", " << Args.size() << ")\n";
+});
+ }
+ OS << "#endif // CLANG_ATTR_NUM_ARGS\n\n";
+}
+
+static bool argSupportsParmPack(const Record *Arg) {
+ return !Arg->getSuperClasses().empty() &&
+ llvm::StringSwitch(
+ Arg->getSuperClasses().back().first->getName())
+ .Case("VariadicExprArgument", true)
+ .Default(false) &&
+ Arg->getValueAsBit("AllowPack");
+}
+
+static void emitClangAttrLastArgParmPackSupport(RecordKeeper ,
+raw_ostream ) {
+ OS << "#if defined(CLANG_ATTR_LAST_ARG_PARM_PACK_SUPPORT)\n";
+ std::vector Attrs = Records.getAllDerivedDefinitions("Attr");
+ for (const auto *A : Attrs) {
+std::vector Args = A->getValueAsListOfDefs("Args");
+bool LastArgSupportsParmPack =
+!Args.empty() && argSupportsParmPack(Args.back());
+forEachUniqueSpelling(*A, [&](const FlattenedSpelling ) {
+ OS << ".Case(\"" << S.name() << "\", " << LastArgSupportsParmPack
+ << ")\n";
+});
+ }
+ OS << "#endif // CLANG_ATTR_LAST_ARG_PARM_PACK_SUPPORT\n\n";
+}
+
+static bool isArgVariadic(const Record , StringRef AttrName) {
+ return createArgument(R, AttrName)->isVariadic();
+}
+
static void emitAttributes(RecordKeeper , raw_ostream ,
bool Header) {
std::vector Attrs = Records.getAllDerivedDefinitions("Attr");
@@ -2320,6 +2363,18 @@
std::vector> Args;
Args.reserve(ArgRecords.size());
+if (!ArgRecords.empty()) {
+ const bool LastArgSupportsPack = argSupportsParmPack(ArgRecords.back());
+ for (size_t I = 0; I < ArgRecords.size() - 1; ++I) {
+assert((!LastArgSupportsPack ||
+!isArgVariadic(*ArgRecords[I], R.getName())) &&
+ "Attributes supporting packs can only have the last "
+ "argument as variadic");
+assert(!argSupportsParmPack(ArgRecords[I]) &&
+ "Only the last argument can allow packs");
+ }
+}
+
bool HasOptArg = false;
bool HasFakeArg = false;
for (const auto *ArgRecord : ArgRecords) {
@@ -3382,10 +3437,6 @@
}
}
-static bool isArgVariadic(const Record , StringRef AttrName) {
- return createArgument(R, AttrName)->isVariadic();
-}
-
static void emitArgInfo(const Record , raw_ostream ) {
// This function will count the number of arguments specified for the
// attribute and emit the number of required arguments followed by the
@@ -4219,6 +4270,8 @@
emitClangAttrIdentifierArgList(Records, OS);
emitClangAttrVariadicIdentifierArgList(Records, OS);
emitClangAttrThisIsaIdentifierArgList(Records, OS);
+ emitClangAttrNumArgs(Records, OS);
+ emitClangAttrLastArgParmPackSupport(Records, OS);
emitClangAttrTypeArgList(Records, OS);
emitClangAttrLateParsedList(Records, OS);
}
Index: clang/test/SemaTemplate/attributes.cpp
===
--- clang/test/SemaTemplate/attributes.cpp
+++ clang/test/SemaTemplate/attributes.cpp
@@ -64,6 +64,23 @@
template [[clang::annotate("ANNOTATE_FOO"), clang::annotate("ANNOTATE_BAR")]] void HasAnnotations();
void UseAnnotations() { HasAnnotations(); }
+// CHECK: FunctionTemplateDecl
[PATCH] D114439: [Annotation] Allow parameter pack expansions in annotate attribute
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 < attrNumArgs) {
aaron.ballman wrote:
> Coding style nits.
We don't typically use top-level `const`, so that should go as well.
Comment at: clang/lib/Parse/ParseExpr.cpp:3374-3375
+if (EarlyTypoCorrection)
+ Expr = Actions.CorrectDelayedTyposInExpr(Expr);
+
steffenlarsen wrote:
> aaron.ballman wrote:
> > steffenlarsen wrote:
> > > aaron.ballman wrote:
> > > > Not that I think this is a bad change, but it seems unrelated to the
> > > > patch. Can you explain why you made the change?
> > > Admittedly I am not sure why it is needed, but in the previous version of
> > > the parsing for attribute parameters it does the typo-correction
> > > immediately after parsing the expression and if this isn't added a
> > > handful of tests fail.
> > Ah, thanks for the info! So this is basically doing the same work as what's
> > done on line 3410 in the late failure case -- I wonder if this should
> > actually be tied to the `FailImmediatelyOnInvalidExpr` parameter instead of
> > having its own parameter?
> They could be unified in the current version, but I don't think there is an
> actual relation between them.
Do we fail tests if this isn't conditionalized at all (we always do the early
typo correction)? (I'd like to avoid adding the new parameter because I'm not
certain how a caller would determine whether they do or do not want that
behavior. If we need the parameter, then so be it, but it seems like this is
something generally useful.)
Comment at: clang/test/Parser/cxx0x-attributes.cpp:262
-template void variadic() {
+template void variadic() {
void bar [[noreturn...]] (); // expected-error {{attribute 'noreturn' cannot
be used as an attribute pack}}
You can back out the whitespace changes for an ever-so-slightly smaller patch.
:-D
Comment at: clang/test/Parser/cxx0x-attributes.cpp:276-277
+ void foz [[clang::annotate("E", 1, 2, 3, Is...)]] ();
+ void fiz [[clang::annotate("F", Is..., 1, 2, 3)]] ();
+ void fir [[clang::annotate("G", 1, Is..., 2, 3)]] ();
+}
Shouldn't these cases also give the `// expected-error {{pack expansion in
'annotate' may only be applied to the last argument}}` diagnostic?
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D114439/new/
https://reviews.llvm.org/D114439
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D114439: [Annotation] Allow parameter pack expansions in annotate attribute
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 to move this comment up closer to the `else` on line 461 > so that it's clear what the else clause covers (and more clear that this is > the common case). I completely agree. It has been moved to right after the `else`. Comment at: clang/lib/Parse/ParseExpr.cpp:3374-3375 +if (EarlyTypoCorrection) + Expr = Actions.CorrectDelayedTyposInExpr(Expr); + aaron.ballman wrote: > steffenlarsen wrote: > > aaron.ballman wrote: > > > Not that I think this is a bad change, but it seems unrelated to the > > > patch. Can you explain why you made the change? > > Admittedly I am not sure why it is needed, but in the previous version of > > the parsing for attribute parameters it does the typo-correction > > immediately after parsing the expression and if this isn't added a handful > > of tests fail. > Ah, thanks for the info! So this is basically doing the same work as what's > done on line 3410 in the late failure case -- I wonder if this should > actually be tied to the `FailImmediatelyOnInvalidExpr` parameter instead of > having its own parameter? They could be unified in the current version, but I don't think there is an actual relation between them. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D114439/new/ https://reviews.llvm.org/D114439 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D114439: [Annotation] Allow parameter pack expansions in annotate attribute
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
clang/include/clang/Parse/Parser.h
clang/lib/Parse/ParseDecl.cpp
clang/lib/Parse/ParseExpr.cpp
clang/lib/Sema/SemaTemplateInstantiateDecl.cpp
clang/test/Parser/cxx0x-attributes.cpp
clang/test/SemaTemplate/attributes.cpp
clang/utils/TableGen/ClangAttrEmitter.cpp
Index: clang/utils/TableGen/ClangAttrEmitter.cpp
===
--- clang/utils/TableGen/ClangAttrEmitter.cpp
+++ clang/utils/TableGen/ClangAttrEmitter.cpp
@@ -1164,10 +1164,12 @@
};
class VariadicExprArgument : public VariadicArgument {
+bool AllowPack = false;
+
public:
VariadicExprArgument(const Record , StringRef Attr)
- : VariadicArgument(Arg, Attr, "Expr *")
-{}
+: VariadicArgument(Arg, Attr, "Expr *"),
+ AllowPack(Arg.getValueAsBit("AllowPack")) {}
void writeASTVisitorTraversal(raw_ostream ) const override {
OS << " {\n";
@@ -2264,6 +2266,47 @@
OS << "#endif // CLANG_ATTR_THIS_ISA_IDENTIFIER_ARG_LIST\n\n";
}
+static void emitClangAttrNumArgs(RecordKeeper , raw_ostream ) {
+ OS << "#if defined(CLANG_ATTR_NUM_ARGS)\n";
+ std::vector Attrs = Records.getAllDerivedDefinitions("Attr");
+ for (const auto *A : Attrs) {
+std::vector Args = A->getValueAsListOfDefs("Args");
+forEachUniqueSpelling(*A, [&](const FlattenedSpelling ) {
+ OS << ".Case(\"" << S.name() << "\", " << Args.size() << ")\n";
+});
+ }
+ OS << "#endif // CLANG_ATTR_NUM_ARGS\n\n";
+}
+
+static bool argSupportsParmPack(const Record *Arg) {
+ return !Arg->getSuperClasses().empty() &&
+ llvm::StringSwitch(
+ Arg->getSuperClasses().back().first->getName())
+ .Case("VariadicExprArgument", true)
+ .Default(false) &&
+ Arg->getValueAsBit("AllowPack");
+}
+
+static void emitClangAttrLastArgParmPackSupport(RecordKeeper ,
+raw_ostream ) {
+ OS << "#if defined(CLANG_ATTR_LAST_ARG_PARM_PACK_SUPPORT)\n";
+ std::vector Attrs = Records.getAllDerivedDefinitions("Attr");
+ for (const auto *A : Attrs) {
+std::vector Args = A->getValueAsListOfDefs("Args");
+bool LastArgSupportsParmPack =
+!Args.empty() && argSupportsParmPack(Args.back());
+forEachUniqueSpelling(*A, [&](const FlattenedSpelling ) {
+ OS << ".Case(\"" << S.name() << "\", " << LastArgSupportsParmPack
+ << ")\n";
+});
+ }
+ OS << "#endif // CLANG_ATTR_LAST_ARG_PARM_PACK_SUPPORT\n\n";
+}
+
+static bool isArgVariadic(const Record , StringRef AttrName) {
+ return createArgument(R, AttrName)->isVariadic();
+}
+
static void emitAttributes(RecordKeeper , raw_ostream ,
bool Header) {
std::vector Attrs = Records.getAllDerivedDefinitions("Attr");
@@ -2320,6 +2363,18 @@
std::vector> Args;
Args.reserve(ArgRecords.size());
+if (!ArgRecords.empty()) {
+ const bool LastArgSupportsPack = argSupportsParmPack(ArgRecords.back());
+ for (size_t I = 0; I < ArgRecords.size() - 1; ++I) {
+assert((!LastArgSupportsPack ||
+!isArgVariadic(*ArgRecords[I], R.getName())) &&
+ "Attributes supporting packs can only have the last "
+ "argument as variadic");
+assert(!argSupportsParmPack(ArgRecords[I]) &&
+ "Only the last argument can allow packs");
+ }
+}
+
bool HasOptArg = false;
bool HasFakeArg = false;
for (const auto *ArgRecord : ArgRecords) {
@@ -3382,10 +3437,6 @@
}
}
-static bool isArgVariadic(const Record , StringRef AttrName) {
- return createArgument(R, AttrName)->isVariadic();
-}
-
static void emitArgInfo(const Record , raw_ostream ) {
// This function will count the number of arguments specified for the
// attribute and emit the number of required arguments followed by the
@@ -4219,6 +4270,8 @@
emitClangAttrIdentifierArgList(Records, OS);
emitClangAttrVariadicIdentifierArgList(Records, OS);
emitClangAttrThisIsaIdentifierArgList(Records, OS);
+ emitClangAttrNumArgs(Records, OS);
+ emitClangAttrLastArgParmPackSupport(Records, OS);
emitClangAttrTypeArgList(Records, OS);
emitClangAttrLateParsedList(Records, OS);
}
Index: clang/test/SemaTemplate/attributes.cpp
===
--- clang/test/SemaTemplate/attributes.cpp
+++ clang/test/SemaTemplate/attributes.cpp
@@ -64,6 +64,23 @@
template [[clang::annotate("ANNOTATE_FOO"), clang::annotate("ANNOTATE_BAR")]] void HasAnnotations();
void UseAnnotations() { HasAnnotations(); }
+// CHECK: FunctionTemplateDecl {{.*}} HasPackAnnotations
+// CHECK:AnnotateAttr {{.*}} "ANNOTATE_BAZ"
+//
[PATCH] D114439: [Annotation] Allow parameter pack expansions in annotate attribute
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 clear what the else clause covers (and more clear that this is the
common case).
Comment at: clang/lib/Parse/ParseDecl.cpp:436-438
+// Interpret "kw_this" as an identifier if the attributed requests it.
+if (ChangeKWThisToIdent && Tok.is(tok::kw_this))
+ Tok.setKind(tok::identifier);
steffenlarsen wrote:
> aaron.ballman wrote:
> > I'm a bit surprised this logic moved -- are there no times when we'd need
> > it within the new `else` clause?
> This is done because `ParseExpressionList` wouldn't know what to do with
> identifiers, so it keeps the old logic. This means that attributes that
> consist of a variadic identifier argument cannot parse packs and initializer
> lists. It shouldn't be a problem for now, but it does kill some generality.
>
> An alternative is to split the individual expression parsing from
> `ParseExpressionList` and use that in here in a similar way to how it parsed
> before. Would that be preferred?
> This is done because ParseExpressionList wouldn't know what to do with
> identifiers, so it keeps the old logic.
Ah, I think I confused myself into thinking `{a, b, c}` would be using a
DeclRefExpr for each of the identifiers, but this code path is for identifiers
that are *not* expressions. Got it.
> An alternative is to split the individual expression parsing from
> ParseExpressionList and use that in here in a similar way to how it parsed
> before. Would that be preferred?
I think this is okay for now.
Comment at: clang/lib/Parse/ParseExpr.cpp:3360
+ llvm::function_ref ExpressionStarts,
+ bool FailImmediatelyOnInvalidExpr,
+ bool EarlyTypoCorrection) {
steffenlarsen wrote:
> aaron.ballman wrote:
> > or something along those lines (note, the logic has reversed meaning).
> > "Fail immediately" doesn't quite convey the behavior to me because we fail
> > immediately either way, it's more about whether we attempt to skip tokens
> > to get to the end of the expression list. I'm not strongly tied to the name
> > I suggested either, btw.
> > "Fail immediately" doesn't quite convey the behavior to me because we fail
> > immediately either way
>
> Am I misunderstanding `FailUntil` or is the intention of
> `SkipUntil(tok::comma, tok::r_paren, StopBeforeMatch)` to either stop at the
> next "," or ")"? If I am not misunderstanding, I believe it will continue
> parsing expressions after comma if `SkipUntil` stopped before it. As such I
> don't think `FailImmediatelyOnInvalidExpr` is inherently wrong, but I agree
> that skipping the skipping isn't very well conveyed by the name as is.
You're correct about the behavior (I had missed that we stopped on a comma as
well as closing paren, I was thinking we were stopping on the closing curly
brace). So the parameter name isn't inherently wrong, just seems a bit unclear
to me. I don't insist on a change, but if someone has a better name for the
parameter, I won't be sad either.
Comment at: clang/lib/Parse/ParseExpr.cpp:3374-3375
+if (EarlyTypoCorrection)
+ Expr = Actions.CorrectDelayedTyposInExpr(Expr);
+
steffenlarsen wrote:
> aaron.ballman wrote:
> > Not that I think this is a bad change, but it seems unrelated to the patch.
> > Can you explain why you made the change?
> Admittedly I am not sure why it is needed, but in the previous version of the
> parsing for attribute parameters it does the typo-correction immediately
> after parsing the expression and if this isn't added a handful of tests fail.
Ah, thanks for the info! So this is basically doing the same work as what's
done on line 3410 in the late failure case -- I wonder if this should actually
be tied to the `FailImmediatelyOnInvalidExpr` parameter instead of having its
own parameter?
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D114439/new/
https://reviews.llvm.org/D114439
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D114439: [Annotation] Allow parameter pack expansions in annotate attribute
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.td
clang/include/clang/Parse/Parser.h
clang/lib/Parse/ParseDecl.cpp
clang/lib/Parse/ParseExpr.cpp
clang/lib/Sema/SemaTemplateInstantiateDecl.cpp
clang/test/Parser/cxx0x-attributes.cpp
clang/test/SemaTemplate/attributes.cpp
clang/utils/TableGen/ClangAttrEmitter.cpp
Index: clang/utils/TableGen/ClangAttrEmitter.cpp
===
--- clang/utils/TableGen/ClangAttrEmitter.cpp
+++ clang/utils/TableGen/ClangAttrEmitter.cpp
@@ -1164,10 +1164,12 @@
};
class VariadicExprArgument : public VariadicArgument {
+bool AllowPack = false;
+
public:
VariadicExprArgument(const Record , StringRef Attr)
- : VariadicArgument(Arg, Attr, "Expr *")
-{}
+: VariadicArgument(Arg, Attr, "Expr *"),
+ AllowPack(Arg.getValueAsBit("AllowPack")) {}
void writeASTVisitorTraversal(raw_ostream ) const override {
OS << " {\n";
@@ -2264,6 +2266,47 @@
OS << "#endif // CLANG_ATTR_THIS_ISA_IDENTIFIER_ARG_LIST\n\n";
}
+static void emitClangAttrNumArgs(RecordKeeper , raw_ostream ) {
+ OS << "#if defined(CLANG_ATTR_NUM_ARGS)\n";
+ std::vector Attrs = Records.getAllDerivedDefinitions("Attr");
+ for (const auto *A : Attrs) {
+std::vector Args = A->getValueAsListOfDefs("Args");
+forEachUniqueSpelling(*A, [&](const FlattenedSpelling ) {
+ OS << ".Case(\"" << S.name() << "\", " << Args.size() << ")\n";
+});
+ }
+ OS << "#endif // CLANG_ATTR_NUM_ARGS\n\n";
+}
+
+static bool argSupportsParmPack(const Record *Arg) {
+ return !Arg->getSuperClasses().empty() &&
+ llvm::StringSwitch(
+ Arg->getSuperClasses().back().first->getName())
+ .Case("VariadicExprArgument", true)
+ .Default(false) &&
+ Arg->getValueAsBit("AllowPack");
+}
+
+static void emitClangAttrLastArgParmPackSupport(RecordKeeper ,
+raw_ostream ) {
+ OS << "#if defined(CLANG_ATTR_LAST_ARG_PARM_PACK_SUPPORT)\n";
+ std::vector Attrs = Records.getAllDerivedDefinitions("Attr");
+ for (const auto *A : Attrs) {
+std::vector Args = A->getValueAsListOfDefs("Args");
+bool LastArgSupportsParmPack =
+!Args.empty() && argSupportsParmPack(Args.back());
+forEachUniqueSpelling(*A, [&](const FlattenedSpelling ) {
+ OS << ".Case(\"" << S.name() << "\", " << LastArgSupportsParmPack
+ << ")\n";
+});
+ }
+ OS << "#endif // CLANG_ATTR_LAST_ARG_PARM_PACK_SUPPORT\n\n";
+}
+
+static bool isArgVariadic(const Record , StringRef AttrName) {
+ return createArgument(R, AttrName)->isVariadic();
+}
+
static void emitAttributes(RecordKeeper , raw_ostream ,
bool Header) {
std::vector Attrs = Records.getAllDerivedDefinitions("Attr");
@@ -2320,6 +2363,18 @@
std::vector> Args;
Args.reserve(ArgRecords.size());
+if (!ArgRecords.empty()) {
+ const bool LastArgSupportsPack = argSupportsParmPack(ArgRecords.back());
+ for (size_t I = 0; I < ArgRecords.size() - 1; ++I) {
+assert((!LastArgSupportsPack ||
+!isArgVariadic(*ArgRecords[I], R.getName())) &&
+ "Attributes supporting packs can only have the last "
+ "argument as variadic");
+assert(!argSupportsParmPack(ArgRecords[I]) &&
+ "Only the last argument can allow packs");
+ }
+}
+
bool HasOptArg = false;
bool HasFakeArg = false;
for (const auto *ArgRecord : ArgRecords) {
@@ -3382,10 +3437,6 @@
}
}
-static bool isArgVariadic(const Record , StringRef AttrName) {
- return createArgument(R, AttrName)->isVariadic();
-}
-
static void emitArgInfo(const Record , raw_ostream ) {
// This function will count the number of arguments specified for the
// attribute and emit the number of required arguments followed by the
@@ -4219,6 +4270,8 @@
emitClangAttrIdentifierArgList(Records, OS);
emitClangAttrVariadicIdentifierArgList(Records, OS);
emitClangAttrThisIsaIdentifierArgList(Records, OS);
+ emitClangAttrNumArgs(Records, OS);
+ emitClangAttrLastArgParmPackSupport(Records, OS);
emitClangAttrTypeArgList(Records, OS);
emitClangAttrLateParsedList(Records, OS);
}
Index: clang/test/SemaTemplate/attributes.cpp
===
--- clang/test/SemaTemplate/attributes.cpp
+++ clang/test/SemaTemplate/attributes.cpp
@@ -64,6 +64,23 @@
template [[clang::annotate("ANNOTATE_FOO"), clang::annotate("ANNOTATE_BAR")]] void HasAnnotations();
void UseAnnotations() { HasAnnotations(); }
+// CHECK: FunctionTemplateDecl {{.*}} HasPackAnnotations
+// CHECK:AnnotateAttr {{.*}}
[PATCH] D114439: [Annotation] Allow parameter pack expansions in annotate attribute
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 (ChangeKWThisToIdent && Tok.is(tok::kw_this))
+ Tok.setKind(tok::identifier);
aaron.ballman wrote:
> I'm a bit surprised this logic moved -- are there no times when we'd need it
> within the new `else` clause?
This is done because `ParseExpressionList` wouldn't know what to do with
identifiers, so it keeps the old logic. This means that attributes that consist
of a variadic identifier argument cannot parse packs and initializer lists. It
shouldn't be a problem for now, but it does kill some generality.
An alternative is to split the individual expression parsing from
`ParseExpressionList` and use that in here in a similar way to how it parsed
before. Would that be preferred?
Comment at: clang/lib/Parse/ParseExpr.cpp:3360
+ llvm::function_ref ExpressionStarts,
+ bool FailImmediatelyOnInvalidExpr,
+ bool EarlyTypoCorrection) {
aaron.ballman wrote:
> or something along those lines (note, the logic has reversed meaning). "Fail
> immediately" doesn't quite convey the behavior to me because we fail
> immediately either way, it's more about whether we attempt to skip tokens to
> get to the end of the expression list. I'm not strongly tied to the name I
> suggested either, btw.
> "Fail immediately" doesn't quite convey the behavior to me because we fail
> immediately either way
Am I misunderstanding `FailUntil` or is the intention of `SkipUntil(tok::comma,
tok::r_paren, StopBeforeMatch)` to either stop at the next "," or ")"? If I am
not misunderstanding, I believe it will continue parsing expressions after
comma if `SkipUntil` stopped before it. As such I don't think
`FailImmediatelyOnInvalidExpr` is inherently wrong, but I agree that skipping
the skipping isn't very well conveyed by the name as is.
Comment at: clang/lib/Parse/ParseExpr.cpp:3374-3375
+if (EarlyTypoCorrection)
+ Expr = Actions.CorrectDelayedTyposInExpr(Expr);
+
aaron.ballman wrote:
> Not that I think this is a bad change, but it seems unrelated to the patch.
> Can you explain why you made the change?
Admittedly I am not sure why it is needed, but in the previous version of the
parsing for attribute parameters it does the typo-correction immediately after
parsing the expression and if this isn't added a handful of tests fail.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D114439/new/
https://reviews.llvm.org/D114439
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D114439: [Annotation] Allow parameter pack expansions in annotate attribute
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_attribute_parm_pack_last_argument_only : Error<
Comment at: clang/include/clang/Basic/DiagnosticParseKinds.td:725
+def err_attribute_parm_pack_last_argument_only : Error<
+ "pack expansion in %0 is restricted to argument %1 or later">;
def err_ms_declspec_type : Error<
Comment at: clang/lib/Parse/ParseDecl.cpp:436-438
+// Interpret "kw_this" as an identifier if the attributed requests it.
+if (ChangeKWThisToIdent && Tok.is(tok::kw_this))
+ Tok.setKind(tok::identifier);
I'm a bit surprised this logic moved -- are there no times when we'd need it
within the new `else` clause?
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 < attrNumArgs) {
Coding style nits.
Comment at: clang/lib/Parse/ParseExpr.cpp:3360
+ llvm::function_ref ExpressionStarts,
+ bool FailImmediatelyOnInvalidExpr,
+ bool EarlyTypoCorrection) {
or something along those lines (note, the logic has reversed meaning). "Fail
immediately" doesn't quite convey the behavior to me because we fail
immediately either way, it's more about whether we attempt to skip tokens to
get to the end of the expression list. I'm not strongly tied to the name I
suggested either, btw.
Comment at: clang/lib/Parse/ParseExpr.cpp:3374-3375
+if (EarlyTypoCorrection)
+ Expr = Actions.CorrectDelayedTyposInExpr(Expr);
+
Not that I think this is a bad change, but it seems unrelated to the patch. Can
you explain why you made the change?
Comment at: clang/test/Parser/cxx0x-attributes.cpp:261
-template void variadic() {
+template void variadic() {
void bar [[noreturn...]] (); // expected-error {{attribute 'noreturn' cannot
be used as an attribute pack}}
steffenlarsen wrote:
> erichkeane wrote:
> > What is the point of this change?
> It makes more sense for the new test cases to get expressions rather than
> types and since `Ts` wasn't used for its contents it did not seem like an
> intrusive change.
Non-type templates are different from type templates, so I don't really want to
lose the test coverage for that (if nothing else, it ensures the parser doesn't
explode when it encounters one). It's fine to add a `variadic_nttp()` function
or something along those lines.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D114439/new/
https://reviews.llvm.org/D114439
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D114439: [Annotation] Allow parameter pack expansions in annotate attribute
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. It also
required some new controlling arguments for `ParseExpressionList`.
Because these changes also allow intializer lists in attribute arguments I have
added a test case showing that `clang::annotate` parses these but will not
accept them.
@erichkeane & @aaron.ballman - Is this in line with what you were thinking?
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
clang/include/clang/Parse/Parser.h
clang/lib/Parse/ParseDecl.cpp
clang/lib/Parse/ParseExpr.cpp
clang/lib/Sema/SemaTemplateInstantiateDecl.cpp
clang/test/Parser/cxx0x-attributes.cpp
clang/test/SemaTemplate/attributes.cpp
clang/utils/TableGen/ClangAttrEmitter.cpp
Index: clang/utils/TableGen/ClangAttrEmitter.cpp
===
--- clang/utils/TableGen/ClangAttrEmitter.cpp
+++ clang/utils/TableGen/ClangAttrEmitter.cpp
@@ -1164,10 +1164,12 @@
};
class VariadicExprArgument : public VariadicArgument {
+bool AllowPack = false;
+
public:
VariadicExprArgument(const Record , StringRef Attr)
- : VariadicArgument(Arg, Attr, "Expr *")
-{}
+: VariadicArgument(Arg, Attr, "Expr *"),
+ AllowPack(Arg.getValueAsBit("AllowPack")) {}
void writeASTVisitorTraversal(raw_ostream ) const override {
OS << " {\n";
@@ -2264,6 +2266,47 @@
OS << "#endif // CLANG_ATTR_THIS_ISA_IDENTIFIER_ARG_LIST\n\n";
}
+static void emitClangAttrNumArgs(RecordKeeper , raw_ostream ) {
+ OS << "#if defined(CLANG_ATTR_NUM_ARGS)\n";
+ std::vector Attrs = Records.getAllDerivedDefinitions("Attr");
+ for (const auto *A : Attrs) {
+std::vector Args = A->getValueAsListOfDefs("Args");
+forEachUniqueSpelling(*A, [&](const FlattenedSpelling ) {
+ OS << ".Case(\"" << S.name() << "\", " << Args.size() << ")\n";
+});
+ }
+ OS << "#endif // CLANG_ATTR_NUM_ARGS\n\n";
+}
+
+static bool argSupportsParmPack(const Record *Arg) {
+ return !Arg->getSuperClasses().empty() &&
+ llvm::StringSwitch(
+ Arg->getSuperClasses().back().first->getName())
+ .Case("VariadicExprArgument", true)
+ .Default(false) &&
+ Arg->getValueAsBit("AllowPack");
+}
+
+static void emitClangAttrLastArgParmPackSupport(RecordKeeper ,
+raw_ostream ) {
+ OS << "#if defined(CLANG_ATTR_LAST_ARG_PARM_PACK_SUPPORT)\n";
+ std::vector Attrs = Records.getAllDerivedDefinitions("Attr");
+ for (const auto *A : Attrs) {
+std::vector Args = A->getValueAsListOfDefs("Args");
+bool LastArgSupportsParmPack =
+!Args.empty() && argSupportsParmPack(Args.back());
+forEachUniqueSpelling(*A, [&](const FlattenedSpelling ) {
+ OS << ".Case(\"" << S.name() << "\", " << LastArgSupportsParmPack
+ << ")\n";
+});
+ }
+ OS << "#endif // CLANG_ATTR_LAST_ARG_PARM_PACK_SUPPORT\n\n";
+}
+
+static bool isArgVariadic(const Record , StringRef AttrName) {
+ return createArgument(R, AttrName)->isVariadic();
+}
+
static void emitAttributes(RecordKeeper , raw_ostream ,
bool Header) {
std::vector Attrs = Records.getAllDerivedDefinitions("Attr");
@@ -2320,6 +2363,18 @@
std::vector> Args;
Args.reserve(ArgRecords.size());
+if (!ArgRecords.empty()) {
+ const bool LastArgSupportsPack = argSupportsParmPack(ArgRecords.back());
+ for (size_t I = 0; I < ArgRecords.size() - 1; ++I) {
+assert((!LastArgSupportsPack ||
+!isArgVariadic(*ArgRecords[I], R.getName())) &&
+ "Attributes supporting packs can only have the last "
+ "argument as variadic");
+assert(!argSupportsParmPack(ArgRecords[I]) &&
+ "Only the last argument can allow packs");
+ }
+}
+
bool HasOptArg = false;
bool HasFakeArg = false;
for (const auto *ArgRecord : ArgRecords) {
@@ -3382,10 +3437,6 @@
}
}
-static bool isArgVariadic(const Record , StringRef AttrName) {
- return createArgument(R, AttrName)->isVariadic();
-}
-
static void emitArgInfo(const Record , raw_ostream ) {
// This function will count the number of arguments specified for the
// attribute and emit the number of required arguments followed by the
@@ -4219,6 +4270,8 @@
emitClangAttrIdentifierArgList(Records, OS);
emitClangAttrVariadicIdentifierArgList(Records, OS);
emitClangAttrThisIsaIdentifierArgList(Records, OS);
+ emitClangAttrNumArgs(Records, OS);
+ emitClangAttrLastArgParmPackSupport(Records, OS);
emitClangAttrTypeArgList(Records, OS);
[PATCH] D114439: [Annotation] Allow parameter pack expansions in annotate attribute
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://github.com/llvm/llvm-project/blob/main/clang/lib/Parse/ParseExpr.cpp#L3059 but on the other hand there could be potential benefits to using `Parser::ParseExpressionList` instead. The primary difference between the behavior of `Parser::ParseExpressionList` and what we are looking for is that it will allow initializer lists as arguments. Since most (if not all) attributes check the validity of their expression arguments, they would likely fail accordingly if they get an initializer list, likely specifying they expected an argument of type X. It may be a blessing in disguise to parse these arguments as well as we are more permissive w.r.t. attribute arguments. Please let me know if there is anything fundamentally wrong with this line of logic. One concern I have is that we currently run `Actions.CorrectDelayedTyposInExpr` on the expression right after parsing it, then we run pack expansion, whereas `Parser::ParseExpressionList` only runs the former action in failure case. I am unsure whether or not that might pose a problem. One problem is that if we only use `Parser::ParseExpressionList` to parse the arguments of the last argument (if it is variadic) then we get the odd behavior that the initializer lists will only be permitted in variadic arguments, which arguably is a little obscure. However, upon further investigation the only case that stops us from using this for all expression arguments are when the attribute has either `VariadicIdentifierArgument` or `VariadicParamOrParamIdxArgument` as the first argument. If we assume that these variadic expressions are limited to the last argument as well (as they are variadic) leaving them as the only argument of the attributes we are parsing here, then we can parse expressions as we did before if we are parsing one of these arguments or switch to using e.g. `Parser::ParseExpressionList` for all expressions of the attribute (even non-variadic) if not. Then we would just have to iterate over the produced expressions to make sure no parameter pack expansions occur before the last (variadic) argument of the parsed attribute. //Summary of suggested changes:// Split attribute parsing into 3: First case parsing a type (currently exists). Second case keeping the current logic for parsing identifiers and single assignment-expressions if the attribute has a single argument of type `VariadicIdentifierArgument` or `VariadicParamOrParamIdxArgument`. Third case, applicable if none of the former ran, parses all expression parameters in one using `Parser::ParseExpressionList` and then checks that parameter packs only occur in the last variadic argument (if it allows it). Side effect is it allows initializer lists in the third case, but attributes are likely to complain about unexpected expression types if these are passed. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D114439/new/ https://reviews.llvm.org/D114439 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D114439: [Annotation] Allow parameter pack expansions in annotate attribute
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:
> > > steffenlarsen wrote:
> > > > erichkeane wrote:
> > > > > steffenlarsen wrote:
> > > > > > erichkeane wrote:
> > > > > > > So I was thinking about this overnight... I wonder if this logic
> > > > > > > is inverted here? Aaron can better answer, but I wonder if we
> > > > > > > should be instead detecting when we are on the 'last' parameter
> > > > > > > and it is one of these `VariadicExprArgument` things (that accept
> > > > > > > a pack), then dropping the parser into a loop of:
> > > > > > >
> > > > > > > while (Tok.is(tok::comma)){
> > > > > > > Tok.Consume();
> > > > > > > ArgExpr = CorrectTypos(ParseAssignmentExpr(...));
> > > > > > > }
> > > > > > > // finally, consume the closing paren.
> > > > > > >
> > > > > > > WDYT?
> > > > > > The parsing of attribute arguments is already this lenient. It does
> > > > > > not actually care how many arguments the attribute says it takes
> > > > > > (unless it takes a type and then it will only currently supply
> > > > > > _one_ argument). It simply consumes as many expressions it can
> > > > > > before reaching the end parenthesis, then leaves the attributes to
> > > > > > consume them in `clang/lib/Sema/SemaDeclAttr.cpp`.
> > > > > > As a result we do not need any additional work here to handle
> > > > > > variadic arguments, but it also means that we have to introduce the
> > > > > > restrictions that we can only allow packs in the last argument and
> > > > > > allow only that argument to be variadic, as otherwise we have no
> > > > > > way of knowing when and where the packs pass between argument
> > > > > > boundaries.
> > > > > My point is, I don't think THIS function should be handling the
> > > > > ellipsis, the expression parsing functions we send the parsing of
> > > > > each component off to should do that.
> > > > >
> > > > > We unfortunately cannot move the entirety of this parsing section to
> > > > > do that, since 'identifier'/'type', and 'Function' argument types end
> > > > > up needing special handling, but it would be nice if our 'expr' types
> > > > > would all be able to do this, and I suggested earlier that we could
> > > > > start with the `VariadicExprArgument` to make this an easier patch
> > > > > that didn't have to deal with the issues that come with that.
> > > > >
> > > > > That said, it would be nice if we could get CLOSE to that.
> > > > Sorry. I am still not sure I am following. Do you mean the consumption
> > > > of pack expressions should be moved into the expression parsing so
> > > > other places (not just attributes) would accept it? If so, I am not
> > > > opposed to the idea though I have no idea which expressions these would
> > > > be nor the full implications of such a change. For these we at least
> > > > have the isolation of having to explicitly support the packs in the
> > > > given attributes.
> > > I'm saying the other expression parsers should ALREADY properly handle
> > > packs, and we should just use those.
> > I think I see what you mean. There is the `Parser::ParseExpressionList`
> > which may fit the bill, though it seems to also accept initializer lists.
> > But since it is an expression the attributes can decide if it is a valid
> > expression for them, so maybe that is not a problem?
> Hmm... I don't have a good idea of that, Aaron has better visibility into
> that sort of thing.
>
> Part of the advantage to doing it the way I suggest is that it ends up being
> a single call (that is, the parser should parse the commas!), so we should
> only have to consume the closing paren.
I think we want something more like `Parser::ParseSimpleExpressionList()`
but... that doesn't handle packs, so those appear to be handled special:
https://github.com/llvm/llvm-project/blob/main/clang/lib/Parse/ParseExpr.cpp#L3059.
But one of these two functions seems to be along the correct lines of what we
want (with modification).
FWIW, I like the sound of Erich's approach better than what's in the current
patch. I'd rather not try to specially handle the ellipsis so much as
recognizing when we expect an arbitrary list of variadic expression arguments
as the last "parameter" for an attribute.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D114439/new/
https://reviews.llvm.org/D114439
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D114439: [Annotation] Allow parameter pack expansions in annotate attribute
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:
> > > erichkeane wrote:
> > > > steffenlarsen wrote:
> > > > > erichkeane wrote:
> > > > > > So I was thinking about this overnight... I wonder if this logic is
> > > > > > inverted here? Aaron can better answer, but I wonder if we should
> > > > > > be instead detecting when we are on the 'last' parameter and it is
> > > > > > one of these `VariadicExprArgument` things (that accept a pack),
> > > > > > then dropping the parser into a loop of:
> > > > > >
> > > > > > while (Tok.is(tok::comma)){
> > > > > > Tok.Consume();
> > > > > > ArgExpr = CorrectTypos(ParseAssignmentExpr(...));
> > > > > > }
> > > > > > // finally, consume the closing paren.
> > > > > >
> > > > > > WDYT?
> > > > > The parsing of attribute arguments is already this lenient. It does
> > > > > not actually care how many arguments the attribute says it takes
> > > > > (unless it takes a type and then it will only currently supply _one_
> > > > > argument). It simply consumes as many expressions it can before
> > > > > reaching the end parenthesis, then leaves the attributes to consume
> > > > > them in `clang/lib/Sema/SemaDeclAttr.cpp`.
> > > > > As a result we do not need any additional work here to handle
> > > > > variadic arguments, but it also means that we have to introduce the
> > > > > restrictions that we can only allow packs in the last argument and
> > > > > allow only that argument to be variadic, as otherwise we have no way
> > > > > of knowing when and where the packs pass between argument boundaries.
> > > > My point is, I don't think THIS function should be handling the
> > > > ellipsis, the expression parsing functions we send the parsing of each
> > > > component off to should do that.
> > > >
> > > > We unfortunately cannot move the entirety of this parsing section to do
> > > > that, since 'identifier'/'type', and 'Function' argument types end up
> > > > needing special handling, but it would be nice if our 'expr' types
> > > > would all be able to do this, and I suggested earlier that we could
> > > > start with the `VariadicExprArgument` to make this an easier patch that
> > > > didn't have to deal with the issues that come with that.
> > > >
> > > > That said, it would be nice if we could get CLOSE to that.
> > > Sorry. I am still not sure I am following. Do you mean the consumption of
> > > pack expressions should be moved into the expression parsing so other
> > > places (not just attributes) would accept it? If so, I am not opposed to
> > > the idea though I have no idea which expressions these would be nor the
> > > full implications of such a change. For these we at least have the
> > > isolation of having to explicitly support the packs in the given
> > > attributes.
> > I'm saying the other expression parsers should ALREADY properly handle
> > packs, and we should just use those.
> I think I see what you mean. There is the `Parser::ParseExpressionList` which
> may fit the bill, though it seems to also accept initializer lists. But since
> it is an expression the attributes can decide if it is a valid expression for
> them, so maybe that is not a problem?
Hmm... I don't have a good idea of that, Aaron has better visibility into that
sort of thing.
Part of the advantage to doing it the way I suggest is that it ends up being a
single call (that is, the parser should parse the commas!), so we should only
have to consume the closing paren.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D114439/new/
https://reviews.llvm.org/D114439
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D114439: [Annotation] Allow parameter pack expansions in annotate attribute
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:
> > > steffenlarsen wrote:
> > > > erichkeane wrote:
> > > > > So I was thinking about this overnight... I wonder if this logic is
> > > > > inverted here? Aaron can better answer, but I wonder if we should be
> > > > > instead detecting when we are on the 'last' parameter and it is one
> > > > > of these `VariadicExprArgument` things (that accept a pack), then
> > > > > dropping the parser into a loop of:
> > > > >
> > > > > while (Tok.is(tok::comma)){
> > > > > Tok.Consume();
> > > > > ArgExpr = CorrectTypos(ParseAssignmentExpr(...));
> > > > > }
> > > > > // finally, consume the closing paren.
> > > > >
> > > > > WDYT?
> > > > The parsing of attribute arguments is already this lenient. It does not
> > > > actually care how many arguments the attribute says it takes (unless it
> > > > takes a type and then it will only currently supply _one_ argument). It
> > > > simply consumes as many expressions it can before reaching the end
> > > > parenthesis, then leaves the attributes to consume them in
> > > > `clang/lib/Sema/SemaDeclAttr.cpp`.
> > > > As a result we do not need any additional work here to handle variadic
> > > > arguments, but it also means that we have to introduce the restrictions
> > > > that we can only allow packs in the last argument and allow only that
> > > > argument to be variadic, as otherwise we have no way of knowing when
> > > > and where the packs pass between argument boundaries.
> > > My point is, I don't think THIS function should be handling the ellipsis,
> > > the expression parsing functions we send the parsing of each component
> > > off to should do that.
> > >
> > > We unfortunately cannot move the entirety of this parsing section to do
> > > that, since 'identifier'/'type', and 'Function' argument types end up
> > > needing special handling, but it would be nice if our 'expr' types would
> > > all be able to do this, and I suggested earlier that we could start with
> > > the `VariadicExprArgument` to make this an easier patch that didn't have
> > > to deal with the issues that come with that.
> > >
> > > That said, it would be nice if we could get CLOSE to that.
> > Sorry. I am still not sure I am following. Do you mean the consumption of
> > pack expressions should be moved into the expression parsing so other
> > places (not just attributes) would accept it? If so, I am not opposed to
> > the idea though I have no idea which expressions these would be nor the
> > full implications of such a change. For these we at least have the
> > isolation of having to explicitly support the packs in the given attributes.
> I'm saying the other expression parsers should ALREADY properly handle packs,
> and we should just use those.
I think I see what you mean. There is the `Parser::ParseExpressionList` which
may fit the bill, though it seems to also accept initializer lists. But since
it is an expression the attributes can decide if it is a valid expression for
them, so maybe that is not a problem?
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D114439/new/
https://reviews.llvm.org/D114439
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D114439: [Annotation] Allow parameter pack expansions in annotate attribute
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:
> > > erichkeane wrote:
> > > > So I was thinking about this overnight... I wonder if this logic is
> > > > inverted here? Aaron can better answer, but I wonder if we should be
> > > > instead detecting when we are on the 'last' parameter and it is one of
> > > > these `VariadicExprArgument` things (that accept a pack), then dropping
> > > > the parser into a loop of:
> > > >
> > > > while (Tok.is(tok::comma)){
> > > > Tok.Consume();
> > > > ArgExpr = CorrectTypos(ParseAssignmentExpr(...));
> > > > }
> > > > // finally, consume the closing paren.
> > > >
> > > > WDYT?
> > > The parsing of attribute arguments is already this lenient. It does not
> > > actually care how many arguments the attribute says it takes (unless it
> > > takes a type and then it will only currently supply _one_ argument). It
> > > simply consumes as many expressions it can before reaching the end
> > > parenthesis, then leaves the attributes to consume them in
> > > `clang/lib/Sema/SemaDeclAttr.cpp`.
> > > As a result we do not need any additional work here to handle variadic
> > > arguments, but it also means that we have to introduce the restrictions
> > > that we can only allow packs in the last argument and allow only that
> > > argument to be variadic, as otherwise we have no way of knowing when and
> > > where the packs pass between argument boundaries.
> > My point is, I don't think THIS function should be handling the ellipsis,
> > the expression parsing functions we send the parsing of each component off
> > to should do that.
> >
> > We unfortunately cannot move the entirety of this parsing section to do
> > that, since 'identifier'/'type', and 'Function' argument types end up
> > needing special handling, but it would be nice if our 'expr' types would
> > all be able to do this, and I suggested earlier that we could start with
> > the `VariadicExprArgument` to make this an easier patch that didn't have to
> > deal with the issues that come with that.
> >
> > That said, it would be nice if we could get CLOSE to that.
> Sorry. I am still not sure I am following. Do you mean the consumption of
> pack expressions should be moved into the expression parsing so other places
> (not just attributes) would accept it? If so, I am not opposed to the idea
> though I have no idea which expressions these would be nor the full
> implications of such a change. For these we at least have the isolation of
> having to explicitly support the packs in the given attributes.
I'm saying the other expression parsers should ALREADY properly handle packs,
and we should just use those.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D114439/new/
https://reviews.llvm.org/D114439
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D114439: [Annotation] Allow parameter pack expansions in annotate attribute
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:
> > > So I was thinking about this overnight... I wonder if this logic is
> > > inverted here? Aaron can better answer, but I wonder if we should be
> > > instead detecting when we are on the 'last' parameter and it is one of
> > > these `VariadicExprArgument` things (that accept a pack), then dropping
> > > the parser into a loop of:
> > >
> > > while (Tok.is(tok::comma)){
> > > Tok.Consume();
> > > ArgExpr = CorrectTypos(ParseAssignmentExpr(...));
> > > }
> > > // finally, consume the closing paren.
> > >
> > > WDYT?
> > The parsing of attribute arguments is already this lenient. It does not
> > actually care how many arguments the attribute says it takes (unless it
> > takes a type and then it will only currently supply _one_ argument). It
> > simply consumes as many expressions it can before reaching the end
> > parenthesis, then leaves the attributes to consume them in
> > `clang/lib/Sema/SemaDeclAttr.cpp`.
> > As a result we do not need any additional work here to handle variadic
> > arguments, but it also means that we have to introduce the restrictions
> > that we can only allow packs in the last argument and allow only that
> > argument to be variadic, as otherwise we have no way of knowing when and
> > where the packs pass between argument boundaries.
> My point is, I don't think THIS function should be handling the ellipsis, the
> expression parsing functions we send the parsing of each component off to
> should do that.
>
> We unfortunately cannot move the entirety of this parsing section to do that,
> since 'identifier'/'type', and 'Function' argument types end up needing
> special handling, but it would be nice if our 'expr' types would all be able
> to do this, and I suggested earlier that we could start with the
> `VariadicExprArgument` to make this an easier patch that didn't have to deal
> with the issues that come with that.
>
> That said, it would be nice if we could get CLOSE to that.
Sorry. I am still not sure I am following. Do you mean the consumption of pack
expressions should be moved into the expression parsing so other places (not
just attributes) would accept it? If so, I am not opposed to the idea though I
have no idea which expressions these would be nor the full implications of such
a change. For these we at least have the isolation of having to explicitly
support the packs in the given attributes.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D114439/new/
https://reviews.llvm.org/D114439
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D114439: [Annotation] Allow parameter pack expansions in annotate attribute
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 about this overnight... I wonder if this logic is
> > inverted here? Aaron can better answer, but I wonder if we should be
> > instead detecting when we are on the 'last' parameter and it is one of
> > these `VariadicExprArgument` things (that accept a pack), then dropping the
> > parser into a loop of:
> >
> > while (Tok.is(tok::comma)){
> > Tok.Consume();
> > ArgExpr = CorrectTypos(ParseAssignmentExpr(...));
> > }
> > // finally, consume the closing paren.
> >
> > WDYT?
> The parsing of attribute arguments is already this lenient. It does not
> actually care how many arguments the attribute says it takes (unless it takes
> a type and then it will only currently supply _one_ argument). It simply
> consumes as many expressions it can before reaching the end parenthesis, then
> leaves the attributes to consume them in `clang/lib/Sema/SemaDeclAttr.cpp`.
> As a result we do not need any additional work here to handle variadic
> arguments, but it also means that we have to introduce the restrictions that
> we can only allow packs in the last argument and allow only that argument to
> be variadic, as otherwise we have no way of knowing when and where the packs
> pass between argument boundaries.
My point is, I don't think THIS function should be handling the ellipsis, the
expression parsing functions we send the parsing of each component off to
should do that.
We unfortunately cannot move the entirety of this parsing section to do that,
since 'identifier'/'type', and 'Function' argument types end up needing special
handling, but it would be nice if our 'expr' types would all be able to do
this, and I suggested earlier that we could start with the
`VariadicExprArgument` to make this an easier patch that didn't have to deal
with the issues that come with that.
That said, it would be nice if we could get CLOSE to that.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D114439/new/
https://reviews.llvm.org/D114439
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D114439: [Annotation] Allow parameter pack expansions in annotate attribute
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... I wonder if this logic is inverted
> here? Aaron can better answer, but I wonder if we should be instead
> detecting when we are on the 'last' parameter and it is one of these
> `VariadicExprArgument` things (that accept a pack), then dropping the parser
> into a loop of:
>
> while (Tok.is(tok::comma)){
> Tok.Consume();
> ArgExpr = CorrectTypos(ParseAssignmentExpr(...));
> }
> // finally, consume the closing paren.
>
> WDYT?
The parsing of attribute arguments is already this lenient. It does not
actually care how many arguments the attribute says it takes (unless it takes a
type and then it will only currently supply _one_ argument). It simply consumes
as many expressions it can before reaching the end parenthesis, then leaves the
attributes to consume them in `clang/lib/Sema/SemaDeclAttr.cpp`.
As a result we do not need any additional work here to handle variadic
arguments, but it also means that we have to introduce the restrictions that we
can only allow packs in the last argument and allow only that argument to be
variadic, as otherwise we have no way of knowing when and where the packs pass
between argument boundaries.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D114439/new/
https://reviews.llvm.org/D114439
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D114439: [Annotation] Allow parameter pack expansions in annotate attribute
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 is inverted
here? Aaron can better answer, but I wonder if we should be instead detecting
when we are on the 'last' parameter and it is one of these
`VariadicExprArgument` things (that accept a pack), then dropping the parser
into a loop of:
while (Tok.is(tok::comma)){
Tok.Consume();
ArgExpr = CorrectTypos(ParseAssignmentExpr(...));
}
// finally, consume the closing paren.
WDYT?
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D114439/new/
https://reviews.llvm.org/D114439
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D114439: [Annotation] Allow parameter pack expansions in annotate attribute
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...)]] ();
}
erichkeane wrote:
> what about:
> void foz [[clang::annotate("D", Is)]] ();
>
> I would expect that to error.
>
> Another test I'd like to see:
>
> void foz[[clang::annotate("E", 1, 2, 3, Is...)]]
>
> Also, I don't see why if THAT works, that:
> void foz[[clang::annotate("E", 1, Is..., 2, 3)]]
>
> shouldn't be allowed as well.
> what about:
> void foz [[clang::annotate("D", Is)]] ();
>
> I would expect that to error.
So would I, but it seems that the parser and annotate is more than happy to
consume this example, even in the current state of Clang:
https://godbolt.org/z/rx64EKeeE
I am not sure as to why, but seeing as it is a preexisting bug I don't know if
it makes sense to include it in the scope of this patch.
>Another test I'd like to see:
>
> void foz[[clang::annotate("E", 1, 2, 3, Is...)]]
>
> Also, I don't see why if THAT works, that:
> void foz[[clang::annotate("E", 1, Is..., 2, 3)]]
>
> shouldn't be allowed as well.
They seem to work as expected. I have added these cases.
Comment at: clang/utils/TableGen/ClangAttrEmitter.cpp:1166
class VariadicExprArgument : public VariadicArgument {
+bool AllowPack = false;
erichkeane wrote:
> The rule of 'only the last argument is allowed to support a pack' should be
> in the attribute emitter.
> The rule of 'only the last argument is allowed to support a pack' should be
> in the attribute emitter.
I have added some assertions around the area where attributes are generate, as
that carries more surrounding information. Is that approximately what you had
in mind?
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D114439/new/
https://reviews.llvm.org/D114439
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D114439: [Annotation] Allow parameter pack expansions in annotate attribute
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 will cause an error if parameter packs are used in the
incorrect argument, specifying which argument should be the earliest for it.
2. Adds additional tests to ensure that variadic arguments supporting packs
also allow pack expansion intermingled with other arguments.
3. Adds assertions to attribute generation to check that attributes only ever
support packs in the last variadic argument and that there are no other
variadic arguments if they do.
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
clang/lib/Parse/ParseDecl.cpp
clang/lib/Sema/SemaTemplateInstantiateDecl.cpp
clang/test/Parser/cxx0x-attributes.cpp
clang/test/SemaTemplate/attributes.cpp
clang/utils/TableGen/ClangAttrEmitter.cpp
Index: clang/utils/TableGen/ClangAttrEmitter.cpp
===
--- clang/utils/TableGen/ClangAttrEmitter.cpp
+++ clang/utils/TableGen/ClangAttrEmitter.cpp
@@ -1164,10 +1164,12 @@
};
class VariadicExprArgument : public VariadicArgument {
+bool AllowPack = false;
+
public:
VariadicExprArgument(const Record , StringRef Attr)
- : VariadicArgument(Arg, Attr, "Expr *")
-{}
+: VariadicArgument(Arg, Attr, "Expr *"),
+ AllowPack(Arg.getValueAsBit("AllowPack")) {}
void writeASTVisitorTraversal(raw_ostream ) const override {
OS << " {\n";
@@ -2264,6 +2266,47 @@
OS << "#endif // CLANG_ATTR_THIS_ISA_IDENTIFIER_ARG_LIST\n\n";
}
+static void emitClangAttrNumArgs(RecordKeeper , raw_ostream ) {
+ OS << "#if defined(CLANG_ATTR_NUM_ARGS)\n";
+ std::vector Attrs = Records.getAllDerivedDefinitions("Attr");
+ for (const auto *A : Attrs) {
+std::vector Args = A->getValueAsListOfDefs("Args");
+forEachUniqueSpelling(*A, [&](const FlattenedSpelling ) {
+ OS << ".Case(\"" << S.name() << "\", " << Args.size() << ")\n";
+});
+ }
+ OS << "#endif // CLANG_ATTR_NUM_ARGS\n\n";
+}
+
+static bool argSupportsParmPack(const Record *Arg) {
+ return !Arg->getSuperClasses().empty() &&
+ llvm::StringSwitch(
+ Arg->getSuperClasses().back().first->getName())
+ .Case("VariadicExprArgument", true)
+ .Default(false) &&
+ Arg->getValueAsBit("AllowPack");
+}
+
+static void emitClangAttrLastArgParmPackSupport(RecordKeeper ,
+raw_ostream ) {
+ OS << "#if defined(CLANG_ATTR_LAST_ARG_PARM_PACK_SUPPORT)\n";
+ std::vector Attrs = Records.getAllDerivedDefinitions("Attr");
+ for (const auto *A : Attrs) {
+std::vector Args = A->getValueAsListOfDefs("Args");
+bool LastArgSupportsParmPack =
+!Args.empty() && argSupportsParmPack(Args.back());
+forEachUniqueSpelling(*A, [&](const FlattenedSpelling ) {
+ OS << ".Case(\"" << S.name() << "\", " << LastArgSupportsParmPack
+ << ")\n";
+});
+ }
+ OS << "#endif // CLANG_ATTR_LAST_ARG_PARM_PACK_SUPPORT\n\n";
+}
+
+static bool isArgVariadic(const Record , StringRef AttrName) {
+ return createArgument(R, AttrName)->isVariadic();
+}
+
static void emitAttributes(RecordKeeper , raw_ostream ,
bool Header) {
std::vector Attrs = Records.getAllDerivedDefinitions("Attr");
@@ -2320,6 +2363,18 @@
std::vector> Args;
Args.reserve(ArgRecords.size());
+if (!ArgRecords.empty()) {
+ const bool LastArgSupportsPack = argSupportsParmPack(ArgRecords.back());
+ for (size_t I = 0; I < ArgRecords.size() - 1; ++I) {
+assert((!LastArgSupportsPack ||
+!isArgVariadic(*ArgRecords[I], R.getName())) &&
+ "Attributes supporting packs can only have the last "
+ "argument as variadic");
+assert(!argSupportsParmPack(ArgRecords[I]) &&
+ "Only the last argument can allow packs");
+ }
+}
+
bool HasOptArg = false;
bool HasFakeArg = false;
for (const auto *ArgRecord : ArgRecords) {
@@ -3382,10 +3437,6 @@
}
}
-static bool isArgVariadic(const Record , StringRef AttrName) {
- return createArgument(R, AttrName)->isVariadic();
-}
-
static void emitArgInfo(const Record , raw_ostream ) {
// This function will count the number of arguments specified for the
// attribute and emit the number of required arguments followed by the
@@ -4219,6 +4270,8 @@
emitClangAttrIdentifierArgList(Records, OS);
emitClangAttrVariadicIdentifierArgList(Records, OS);
emitClangAttrThisIsaIdentifierArgList(Records, OS);
+ emitClangAttrNumArgs(Records, OS);
+ emitClangAttrLastArgParmPackSupport(Records,
[PATCH] D114439: [Annotation] Allow parameter pack expansions in annotate attribute
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 to only the last argument">;
erichkeane wrote:
> I don't really see why this is required? I would think the 722 would be all
> you would need.
Intention was to make a distinction between the two cases, since we had the
granularity to do so. I.e. `clang::annotate` would never use 722 as it
explicitly states that the attribute doesn't support pack expansion (in the
last argument) which is untrue for `clang::annotate`, but if a user was to do
pack expansion on the first argument of `clang::annotate` they would get this
error telling them that the given argument cannot accept the pack expansion.
Comment at: clang/lib/Parse/ParseDecl.cpp:450
+ // Pack expansion is only allowed in the last attribute argument.
+ if (ArgExprs.size() + 1 < attributeNumberOfArguments(*AttrName)) {
+Diag(Tok.getLocation(),
erichkeane wrote:
> I don't think this should be diagnosed here, and I don't think it is 'right'.
> I think the ClangAttrEmitter should ensure that the VariadicExprArgument
> needs to be the 'last' thing, but I think that REALLY means "supports a pack
> anywhere inside of it".
>
> See my test examples below, I don't think this parsing is sufficient for that.
That is also the intention here. All it checks is that we are in or beyond the
last argument of the attribute. For example, `clang::annotate` will return 2
from `attributeNumberOfArguments` as `VariadicExprArgument` only counts as a
single argument. Thus, any pack expansion expressions parsed after the first
will be accepted. I do agree though that the error message may be a little
confusing for users.
I will add the suggested tests and rethink the diagnostics.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D114439/new/
https://reviews.llvm.org/D114439
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D114439: [Annotation] Allow parameter pack expansions in annotate attribute
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 only the last argument">;
I don't really see why this is required? I would think the 722 would be all
you would need.
Comment at: clang/lib/Parse/ParseDecl.cpp:450
+ // Pack expansion is only allowed in the last attribute argument.
+ if (ArgExprs.size() + 1 < attributeNumberOfArguments(*AttrName)) {
+Diag(Tok.getLocation(),
I don't think this should be diagnosed here, and I don't think it is 'right'.
I think the ClangAttrEmitter should ensure that the VariadicExprArgument needs
to be the 'last' thing, but I think that REALLY means "supports a pack anywhere
inside of it".
See my test examples below, I don't think this parsing is sufficient for that.
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...)]] ();
}
what about:
void foz [[clang::annotate("D", Is)]] ();
I would expect that to error.
Another test I'd like to see:
void foz[[clang::annotate("E", 1, 2, 3, Is...)]]
Also, I don't see why if THAT works, that:
void foz[[clang::annotate("E", 1, Is..., 2, 3)]]
shouldn't be allowed as well.
Comment at: clang/utils/TableGen/ClangAttrEmitter.cpp:1166
class VariadicExprArgument : public VariadicArgument {
+bool AllowPack = false;
The rule of 'only the last argument is allowed to support a pack' should be in
the attribute emitter.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D114439/new/
https://reviews.llvm.org/D114439
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D114439: [Annotation] Allow parameter pack expansions in annotate attribute
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 requested test cases. Please have a look and let me know what you think. > That's what I want to avoid. :-D I would prefer that the arguments have to > opt into that behavior because the alternative could get ambiguous. I'd > rather we do some extra work to explicitly support that situation once we > have a specific attribute in mind for it. I'm okay with that! I have made the changes to only allow it in the last argument if it is marked as supporting pack expansion. Note that it assumes that there are no other variadic parameters except the last one, as suggested. > Hmm, let's make sure we're on the same page. The situations I think we should > avoid are: > > // Mixing variadics and packs. > let Args = [VariadicExprArgument<"VarArgs">, VariadicExprArgument<"Pack", > /*AllowPack*/1>]; > > // Multiple packs. > let Args = [VariadicExprArgument<"FirstPack", /*AllowPack*/1>, > VariadicExprArgument<"SecondPack", /*AllowPack*/1>]; Oh, I see what you mean. Yeah I agree for sure. I think the only exceptions we currently have to this is `omp` attributes, but given that they are pragmas they should be nowhere close to this. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D114439/new/ https://reviews.llvm.org/D114439 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D114439: [Annotation] Allow parameter pack expansions in annotate attribute
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
clang/lib/Parse/ParseDecl.cpp
clang/lib/Sema/SemaTemplateInstantiateDecl.cpp
clang/test/Parser/cxx0x-attributes.cpp
clang/test/SemaTemplate/attributes.cpp
clang/utils/TableGen/ClangAttrEmitter.cpp
Index: clang/utils/TableGen/ClangAttrEmitter.cpp
===
--- clang/utils/TableGen/ClangAttrEmitter.cpp
+++ clang/utils/TableGen/ClangAttrEmitter.cpp
@@ -1164,10 +1164,12 @@
};
class VariadicExprArgument : public VariadicArgument {
+bool AllowPack = false;
+
public:
VariadicExprArgument(const Record , StringRef Attr)
- : VariadicArgument(Arg, Attr, "Expr *")
-{}
+: VariadicArgument(Arg, Attr, "Expr *"),
+ AllowPack(Arg.getValueAsBit("AllowPack")) {}
void writeASTVisitorTraversal(raw_ostream ) const override {
OS << " {\n";
@@ -2264,6 +2266,43 @@
OS << "#endif // CLANG_ATTR_THIS_ISA_IDENTIFIER_ARG_LIST\n\n";
}
+static void emitClangAttrNumArgs(RecordKeeper , raw_ostream ) {
+ OS << "#if defined(CLANG_ATTR_NUM_ARGS)\n";
+ std::vector Attrs = Records.getAllDerivedDefinitions("Attr");
+ for (const auto *A : Attrs) {
+std::vector Args = A->getValueAsListOfDefs("Args");
+forEachUniqueSpelling(*A, [&](const FlattenedSpelling ) {
+ OS << ".Case(\"" << S.name() << "\", " << Args.size() << ")\n";
+});
+ }
+ OS << "#endif // CLANG_ATTR_NUM_ARGS\n\n";
+}
+
+static bool argSupportsParmPack(const Record *Arg) {
+ return !Arg->getSuperClasses().empty() &&
+ llvm::StringSwitch(
+ Arg->getSuperClasses().back().first->getName())
+ .Case("VariadicExprArgument", true)
+ .Default(false) &&
+ Arg->getValueAsBit("AllowPack");
+}
+
+static void emitClangAttrLastArgParmPackSupport(RecordKeeper ,
+raw_ostream ) {
+ OS << "#if defined(CLANG_ATTR_LAST_ARG_PARM_PACK_SUPPORT)\n";
+ std::vector Attrs = Records.getAllDerivedDefinitions("Attr");
+ for (const auto *A : Attrs) {
+std::vector Args = A->getValueAsListOfDefs("Args");
+bool LastArgSupportsParmPack =
+!Args.empty() && argSupportsParmPack(Args.back());
+forEachUniqueSpelling(*A, [&](const FlattenedSpelling ) {
+ OS << ".Case(\"" << S.name() << "\", " << LastArgSupportsParmPack
+ << ")\n";
+});
+ }
+ OS << "#endif // CLANG_ATTR_LAST_ARG_PARM_PACK_SUPPORT\n\n";
+}
+
static void emitAttributes(RecordKeeper , raw_ostream ,
bool Header) {
std::vector Attrs = Records.getAllDerivedDefinitions("Attr");
@@ -4219,6 +4258,8 @@
emitClangAttrIdentifierArgList(Records, OS);
emitClangAttrVariadicIdentifierArgList(Records, OS);
emitClangAttrThisIsaIdentifierArgList(Records, OS);
+ emitClangAttrNumArgs(Records, OS);
+ emitClangAttrLastArgParmPackSupport(Records, OS);
emitClangAttrTypeArgList(Records, OS);
emitClangAttrLateParsedList(Records, OS);
}
Index: clang/test/SemaTemplate/attributes.cpp
===
--- clang/test/SemaTemplate/attributes.cpp
+++ clang/test/SemaTemplate/attributes.cpp
@@ -64,6 +64,23 @@
template [[clang::annotate("ANNOTATE_FOO"), clang::annotate("ANNOTATE_BAR")]] void HasAnnotations();
void UseAnnotations() { HasAnnotations(); }
+// CHECK: FunctionTemplateDecl {{.*}} HasPackAnnotations
+// CHECK:AnnotateAttr {{.*}} "ANNOTATE_BAZ"
+// CHECK: FunctionDecl {{.*}} HasPackAnnotations
+// CHECK:TemplateArgument{{.*}} pack
+// CHECK-NEXT: TemplateArgument{{.*}} integral 1
+// CHECK-NEXT: TemplateArgument{{.*}} integral 2
+// CHECK-NEXT: TemplateArgument{{.*}} integral 3
+// CHECK:AnnotateAttr {{.*}} "ANNOTATE_BAZ"
+// CHECK: ConstantExpr {{.*}} 'int'
+// CHECK-NEXT: value: Int 1
+// CHECK: ConstantExpr {{.*}} 'int'
+// CHECK-NEXT: value: Int 2
+// CHECK: ConstantExpr {{.*}} 'int'
+// CHECK-NEXT: value: Int 3
+template [[clang::annotate("ANNOTATE_BAZ", Is...)]] void HasPackAnnotations();
+void UsePackAnnotations() { HasPackAnnotations<1, 2, 3>(); }
+
namespace preferred_name {
int x [[clang::preferred_name("frank")]]; // expected-error {{expected a type}}
int y [[clang::preferred_name(int)]]; // expected-warning {{'preferred_name' attribute only applies to class templates}}
Index: clang/test/Parser/cxx0x-attributes.cpp
===
--- clang/test/Parser/cxx0x-attributes.cpp
+++ clang/test/Parser/cxx0x-attributes.cpp
@@ -258,8 +258,14 @@
[[]] return;
}
-template void variadic() {
- void bar [[noreturn...]] (); // expected-error
[PATCH] D114439: [Annotation] Allow parameter pack expansions in annotate attribute
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 wrote: > > > From a design perspective, I think this is a property of the parameters > > > of an attribute rather than of an attribute itself. So I'd rather not add > > > a bit to the `Attr` class, but instead modify the `Argument` class. There > > > are a few approaches to that which I can think of, and I'm not certain > > > which is the best approach. > > > > > > 1) Introduce `ExpressionPackArgument` and attributes can opt into using > > > it if they want to support packs. > > > 2) Add this bit to `VariadicExprArgument` and attributes can opt into > > > using it if they want to support packs. > > > 3) Change `VariadicExprArgument` to also mean "allows packs" and all > > > attributes using this automatically support packs. > > > > > > I think that my conservative preference is for #2, but my intuition is > > > that #3 is where we probably ultimately want to get to. > > > > > > There are definitely some open questions with this approach that we'll > > > have to figure out: > > > > > > * Can you mix packs with variadics in the same attribute? > > > * Can you have two pack arguments in the same attribute? > > > * Does the tablegen design also seem likely to work when we want to > > > introduce type packs instead of value packs? > > > > > > 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 FIXME comment to that > > > diagnostic logic asking whether we want to relax the behavior at some > > > point. But if you feel strongly that we should support these situations > > > initially, I wouldn't be opposed. > > Having `ParmExpansionArgsSupport` in `Attr` would allow attributes to > > populate with parameter packs across argument boundaries. It seems more > > permissive, but frankly I have no attachment to it, so I am okay with tying > > it to `Argument` instead. > > > > Of the mentioned options, I agree #2 and #3 are the preferred paths. For > > simplicity I prefer #2, though I will have to figure out how best to add a > > check on individual arguments, which @erichkeane's suggestion to limit the > > attributes to only allowing parameter packs in variadic arguments as the > > last argument would help with. On the other hand, the population logic for > > most of the relevant attributes are explicitly defined in > > `clang/lib/Sema/SemaDeclAttr.cpp` (which will have to be changed with #3) > > so being more permissive - like the current version of the patch is - gives > > more freedom to the attributes. > > > > > Does the tablegen design also seem likely to work when we want to > > > introduce type packs instead of value packs? > > > > I am not sure about how tablegen would take it, but the parser doesn't > > currently parse more than a single type argument, so I don't see a need for > > allowing type packs at the moment. > > > > > 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 FIXME comment to that > > > diagnostic logic asking whether we want to relax the behavior at some > > > point. But if you feel strongly that we should support these situations > > > initially, I wouldn't be opposed. > > > > If we limit packs to `VariadicExprArgument` - like `annotate` does in the > > current state of this patch - having multiple packs in the same argument is > > simple to handle. Both are represented as `Expr` and the variadic argument > > will simply contain two expressions until the templates are instantiated > > and the packs are expanded. This is also a feature I would like to keep, > > though it should be possible to work around the limitation if we conclude > > it can't stay. > > > > Having ParmExpansionArgsSupport in Attr would allow attributes to populate > > with parameter packs across argument boundaries. > > That's what I want to avoid. :-D I would prefer that the arguments have to > opt into that behavior because the alternative could get ambiguous. I'd > rather we do some extra work to explicitly support that situation once we > have a specific attribute in mind for it. > > > ... which @erichkeane's suggestion to limit the attributes to only allowing > > parameter packs in variadic arguments as the last argument would help with. > > I agree with that suggestion, btw. > > > I am not sure about how
[PATCH] D114439: [Annotation] Allow parameter pack expansions in annotate attribute
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 wrote: > > From a design perspective, I think this is a property of the parameters of > > an attribute rather than of an attribute itself. So I'd rather not add a > > bit to the `Attr` class, but instead modify the `Argument` class. There are > > a few approaches to that which I can think of, and I'm not certain which is > > the best approach. > > > > 1) Introduce `ExpressionPackArgument` and attributes can opt into using it > > if they want to support packs. > > 2) Add this bit to `VariadicExprArgument` and attributes can opt into using > > it if they want to support packs. > > 3) Change `VariadicExprArgument` to also mean "allows packs" and all > > attributes using this automatically support packs. > > > > I think that my conservative preference is for #2, but my intuition is that > > #3 is where we probably ultimately want to get to. > > > > There are definitely some open questions with this approach that we'll have > > to figure out: > > > > * Can you mix packs with variadics in the same attribute? > > * Can you have two pack arguments in the same attribute? > > * Does the tablegen design also seem likely to work when we want to > > introduce type packs instead of value packs? > > > > 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 FIXME comment to that diagnostic logic > > asking whether we want to relax the behavior at some point. But if you feel > > strongly that we should support these situations initially, I wouldn't be > > opposed. > Having `ParmExpansionArgsSupport` in `Attr` would allow attributes to > populate with parameter packs across argument boundaries. It seems more > permissive, but frankly I have no attachment to it, so I am okay with tying > it to `Argument` instead. > > Of the mentioned options, I agree #2 and #3 are the preferred paths. For > simplicity I prefer #2, though I will have to figure out how best to add a > check on individual arguments, which @erichkeane's suggestion to limit the > attributes to only allowing parameter packs in variadic arguments as the last > argument would help with. On the other hand, the population logic for most of > the relevant attributes are explicitly defined in > `clang/lib/Sema/SemaDeclAttr.cpp` (which will have to be changed with #3) so > being more permissive - like the current version of the patch is - gives more > freedom to the attributes. > > > Does the tablegen design also seem likely to work when we want to introduce > > type packs instead of value packs? > > I am not sure about how tablegen would take it, but the parser doesn't > currently parse more than a single type argument, so I don't see a need for > allowing type packs at the moment. > > > 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 FIXME comment to that diagnostic logic > > asking whether we want to relax the behavior at some point. But if you feel > > strongly that we should support these situations initially, I wouldn't be > > opposed. > > If we limit packs to `VariadicExprArgument` - like `annotate` does in the > current state of this patch - having multiple packs in the same argument is > simple to handle. Both are represented as `Expr` and the variadic argument > will simply contain two expressions until the templates are instantiated and > the packs are expanded. This is also a feature I would like to keep, though > it should be possible to work around the limitation if we conclude it can't > stay. > > Having ParmExpansionArgsSupport in Attr would allow attributes to populate > with parameter packs across argument boundaries. That's what I want to avoid. :-D I would prefer that the arguments have to opt into that behavior because the alternative could get ambiguous. I'd rather we do some extra work to explicitly support that situation once we have a specific attribute in mind for it. > ... which @erichkeane's suggestion to limit the attributes to only allowing > parameter packs in variadic arguments as the last argument would help with. I agree with that suggestion, btw. > I am not sure about how tablegen would take it, but the parser doesn't > currently parse more than a single type argument, so I don't see a need for > allowing type packs at the moment. Fair point, and yet
[PATCH] D114439: [Annotation] Allow parameter pack expansions in annotate attribute
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 something that isn't a pack followed by an ellipsis? > > What is the behavior there? I'm also concerned that there doesn't seem > > to be anything here that handles complex-pack expansions (like folds), can > > you test that as well? > > > > Also, why is this 'if' not up on line 429 (that is, outside of this > > 'else'?). I believe ParseAssignmentExpression is going to muck-up the > > tokens, so I'm not particularly sure what is going to happen here? > > Do you have a test for something that isn't a pack followed by an ellipsis? > > What is the behavior there? I'm also concerned that there doesn't seem to > > be anything here that handles complex-pack expansions (like folds), can you > > test that as well? > > Good question! I would expect it to fail in kind during template > instantiation, or earlier if it is not an expression, but I will test it out > once we converge on a new solution. > > > Also, why is this 'if' not up on line 429 (that is, outside of this > > 'else'?). I believe ParseAssignmentExpression is going to muck-up the > > tokens, so I'm not particularly sure what is going to happen here? > > I originally tried having the `ActOnPackExpansion` earlier, but it did indeed > cause trouble. However, if my understanding of the previous two branches is > correct, they are: > > > # Type argument. This could be consuming the ellipsis as well, though I > suspect it needs another path given that it does not seem to produce any > expressions. Note however that it currently seems to stop argument parsing > after reading the first type (confirmed by the FIXME) so parameter pack logic > for this branch would probably be more suitable once it actually allows > attributes to take multiple types. > # Identifier argument. Correct me if I am wrong, but I don't think this can > be populated by a parameter pack. > > I could add the diagnostic to these if ellipsis is found, but if my > assumptions are correct it doesn't make much sense to expand these branches > beyond that. > I'm also concerned that there doesn't seem to be anything here that handles > complex-pack expansions (like folds), can you test that as well? Sorry, I forgot to address this. I have not tested it, but I would expect complex-pack expansions like folds to be at expression-level. I will make sure to test this as well. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D114439/new/ https://reviews.llvm.org/D114439 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D114439: [Annotation] Allow parameter pack expansions in annotate attribute
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 perspective, I think this is a property of the parameters of an > attribute rather than of an attribute itself. So I'd rather not add a bit to > the `Attr` class, but instead modify the `Argument` class. There are a few > approaches to that which I can think of, and I'm not certain which is the > best approach. > > 1) Introduce `ExpressionPackArgument` and attributes can opt into using it if > they want to support packs. > 2) Add this bit to `VariadicExprArgument` and attributes can opt into using > it if they want to support packs. > 3) Change `VariadicExprArgument` to also mean "allows packs" and all > attributes using this automatically support packs. > > I think that my conservative preference is for #2, but my intuition is that > #3 is where we probably ultimately want to get to. > > There are definitely some open questions with this approach that we'll have > to figure out: > > * Can you mix packs with variadics in the same attribute? > * Can you have two pack arguments in the same attribute? > * Does the tablegen design also seem likely to work when we want to introduce > type packs instead of value packs? > > 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 FIXME comment to that diagnostic logic asking > whether we want to relax the behavior at some point. But if you feel strongly > that we should support these situations initially, I wouldn't be opposed. Having `ParmExpansionArgsSupport` in `Attr` would allow attributes to populate with parameter packs across argument boundaries. It seems more permissive, but frankly I have no attachment to it, so I am okay with tying it to `Argument` instead. Of the mentioned options, I agree #2 and #3 are the preferred paths. For simplicity I prefer #2, though I will have to figure out how best to add a check on individual arguments, which @erichkeane's suggestion to limit the attributes to only allowing parameter packs in variadic arguments as the last argument would help with. On the other hand, the population logic for most of the relevant attributes are explicitly defined in `clang/lib/Sema/SemaDeclAttr.cpp` (which will have to be changed with #3) so being more permissive - like the current version of the patch is - gives more freedom to the attributes. > Does the tablegen design also seem likely to work when we want to introduce > type packs instead of value packs? I am not sure about how tablegen would take it, but the parser doesn't currently parse more than a single type argument, so I don't see a need for allowing type packs at the moment. > 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 FIXME comment to that diagnostic logic asking > whether we want to relax the behavior at some point. But if you feel strongly > that we should support these situations initially, I wouldn't be opposed. If we limit packs to `VariadicExprArgument` - like `annotate` does in the current state of this patch - having multiple packs in the same argument is simple to handle. Both are represented as `Expr` and the variadic argument will simply contain two expressions until the templates are instantiated and the packs are expanded. This is also a feature I would like to keep, though it should be possible to work around the limitation if we conclude it can't stay. Comment at: clang/lib/Parse/ParseDecl.cpp:447 + } + ArgExpr = Actions.ActOnPackExpansion(ArgExpr.get(), ConsumeToken()); +} erichkeane wrote: > Do you have a test for something that isn't a pack followed by an ellipsis? > What is the behavior there? I'm also concerned that there doesn't seem to > be anything here that handles complex-pack expansions (like folds), can you > test that as well? > > Also, why is this 'if' not up on line 429 (that is, outside of this 'else'?). > I believe ParseAssignmentExpression is going to muck-up the tokens, so I'm > not particularly sure what is going to happen here? > Do you have a test for something that isn't a pack followed by an ellipsis? > What is the behavior there? I'm also concerned that there doesn't seem to be > anything here that handles complex-pack expansions (like folds), can you test > that as well? Good question! I would expect it to fail
[PATCH] D114439: [Annotation] Allow parameter pack expansions in annotate attribute
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 FIXME comment to that diagnostic logic >> asking whether we want to relax the behavior at some point. But if you feel >> strongly that we should support these situations initially, I wouldn't be >> opposed. From a practical perspective, the only difference between a list of `ExprArgument` and `VariadicExprArgument` is the enforced arity I would also consider seeing if we can just generalize the pack logic to work for BOTH cases. I'd be all for limiting the pack/ExprArgument list to be the 'last' argument to simplify it. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D114439/new/ https://reviews.llvm.org/D114439 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D114439: [Annotation] Allow parameter pack expansions in annotate attribute
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 + // arguments. + bit ParmExpansionArgsSupport = 0; From a design perspective, I think this is a property of the parameters of an attribute rather than of an attribute itself. So I'd rather not add a bit to the `Attr` class, but instead modify the `Argument` class. There are a few approaches to that which I can think of, and I'm not certain which is the best approach. 1) Introduce `ExpressionPackArgument` and attributes can opt into using it if they want to support packs. 2) Add this bit to `VariadicExprArgument` and attributes can opt into using it if they want to support packs. 3) Change `VariadicExprArgument` to also mean "allows packs" and all attributes using this automatically support packs. I think that my conservative preference is for #2, but my intuition is that #3 is where we probably ultimately want to get to. There are definitely some open questions with this approach that we'll have to figure out: * Can you mix packs with variadics in the same attribute? * Can you have two pack arguments in the same attribute? * Does the tablegen design also seem likely to work when we want to introduce type packs instead of value packs? 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 FIXME comment to that diagnostic logic asking whether we want to relax the behavior at some point. But if you feel strongly that we should support these situations initially, I wouldn't be opposed. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D114439/new/ https://reviews.llvm.org/D114439 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D114439: [Annotation] Allow parameter pack expansions in annotate attribute
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
+ }
+ ArgExpr = Actions.ActOnPackExpansion(ArgExpr.get(), ConsumeToken());
+}
Do you have a test for something that isn't a pack followed by an ellipsis?
What is the behavior there? I'm also concerned that there doesn't seem to be
anything here that handles complex-pack expansions (like folds), can you test
that as well?
Also, why is this 'if' not up on line 429 (that is, outside of this 'else'?).
I believe ParseAssignmentExpression is going to muck-up the tokens, so I'm not
particularly sure what is going to happen here?
Comment at: clang/test/Parser/cxx0x-attributes.cpp:261
-template void variadic() {
+template void variadic() {
void bar [[noreturn...]] (); // expected-error {{attribute 'noreturn' cannot
be used as an attribute pack}}
What is the point of this change?
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D114439/new/
https://reviews.llvm.org/D114439
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D114439: [Annotation] Allow parameter pack expansions in annotate attribute
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-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D114439: [Annotation] Allow parameter pack expansions in annotate attribute
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 parser recognize parameter pack expansion in
attribute arguments and consume them if the attribute is marked as supporting
parameter pack expansions.
Currently only the `clang::annotate` attribute will support parameter pack
expansions in its arguments. The parser will issue an error diagnostic if other
attributes are passed parameter pack expansion arguments.
Repository:
rG LLVM Github Monorepo
https://reviews.llvm.org/D114439
Files:
clang/include/clang/Basic/Attr.td
clang/include/clang/Basic/DiagnosticParseKinds.td
clang/lib/Parse/ParseDecl.cpp
clang/lib/Sema/SemaTemplateInstantiateDecl.cpp
clang/test/Parser/cxx0x-attributes.cpp
clang/test/SemaTemplate/attributes.cpp
clang/utils/TableGen/ClangAttrEmitter.cpp
Index: clang/utils/TableGen/ClangAttrEmitter.cpp
===
--- clang/utils/TableGen/ClangAttrEmitter.cpp
+++ clang/utils/TableGen/ClangAttrEmitter.cpp
@@ -1697,6 +1697,22 @@
OS << "#endif // CLANG_ATTR_LATE_PARSED_LIST\n\n";
}
+// Emits the ParmExpansionArgsSupport property for attributes.
+static void emitClangAttrParmExpansionArgsSupportList(RecordKeeper ,
+ raw_ostream ) {
+ OS << "#if defined(CLANG_ATTR_PARM_EXPANSION_ARGS_SUPPORT_LIST)\n";
+ std::vector Attrs = Records.getAllDerivedDefinitions("Attr");
+
+ for (const auto *Attr : Attrs) {
+if (Attr->getValueAsBit("ParmExpansionArgsSupport")) {
+ std::vector Spellings = GetFlattenedSpellings(*Attr);
+ for (const auto : Spellings)
+OS << ".Case(\"" << I.name() << "\", true)\n";
+}
+ }
+ OS << "#endif // CLANG_ATTR_PARM_EXPANSION_ARGS_SUPPORT_LIST\n\n";
+}
+
static bool hasGNUorCXX11Spelling(const Record ) {
std::vector Spellings = GetFlattenedSpellings(Attribute);
for (const auto : Spellings) {
@@ -4221,6 +4237,7 @@
emitClangAttrThisIsaIdentifierArgList(Records, OS);
emitClangAttrTypeArgList(Records, OS);
emitClangAttrLateParsedList(Records, OS);
+ emitClangAttrParmExpansionArgsSupportList(Records, OS);
}
void EmitClangAttrSubjectMatchRulesParserStringSwitches(RecordKeeper ,
Index: clang/test/SemaTemplate/attributes.cpp
===
--- clang/test/SemaTemplate/attributes.cpp
+++ clang/test/SemaTemplate/attributes.cpp
@@ -64,6 +64,23 @@
template [[clang::annotate("ANNOTATE_FOO"), clang::annotate("ANNOTATE_BAR")]] void HasAnnotations();
void UseAnnotations() { HasAnnotations(); }
+// CHECK: FunctionTemplateDecl {{.*}} HasPackAnnotations
+// CHECK:AnnotateAttr {{.*}} "ANNOTATE_BAZ"
+// CHECK: FunctionDecl {{.*}} HasPackAnnotations
+// CHECK:TemplateArgument{{.*}} pack
+// CHECK-NEXT: TemplateArgument{{.*}} integral 1
+// CHECK-NEXT: TemplateArgument{{.*}} integral 2
+// CHECK-NEXT: TemplateArgument{{.*}} integral 3
+// CHECK:AnnotateAttr {{.*}} "ANNOTATE_BAZ"
+// CHECK: ConstantExpr {{.*}} 'int'
+// CHECK-NEXT: value: Int 1
+// CHECK: ConstantExpr {{.*}} 'int'
+// CHECK-NEXT: value: Int 2
+// CHECK: ConstantExpr {{.*}} 'int'
+// CHECK-NEXT: value: Int 3
+template [[clang::annotate("ANNOTATE_BAZ", Is...)]] void HasPackAnnotations();
+void UsePackAnnotations() { HasPackAnnotations<1, 2, 3>(); }
+
namespace preferred_name {
int x [[clang::preferred_name("frank")]]; // expected-error {{expected a type}}
int y [[clang::preferred_name(int)]]; // expected-warning {{'preferred_name' attribute only applies to class templates}}
Index: clang/test/Parser/cxx0x-attributes.cpp
===
--- clang/test/Parser/cxx0x-attributes.cpp
+++ clang/test/Parser/cxx0x-attributes.cpp
@@ -258,8 +258,10 @@
[[]] return;
}
-template void variadic() {
+template void variadic() {
void bar [[noreturn...]] (); // expected-error {{attribute 'noreturn' cannot be used as an attribute pack}}
+ void baz [[clang::no_sanitize(Is...)]] (); // expected-error {{attribute 'no_sanitize' does not support parameter pack expansion in arguments}}
+ void boo [[unknown::foo(Is...)]] (); // expected-warning {{unknown attribute 'foo' ignored}}
}
// Expression tests
Index: clang/lib/Sema/SemaTemplateInstantiateDecl.cpp
===
--- clang/lib/Sema/SemaTemplateInstantiateDecl.cpp
+++ clang/lib/Sema/SemaTemplateInstantiateDecl.cpp
@@ -189,13 +189,9 @@
EnterExpressionEvaluationContext Unevaluated(
S, Sema::ExpressionEvaluationContext::ConstantEvaluated);
SmallVector Args;
- Args.reserve(Attr->args_size());
- for
