[PATCH] D114439: [Annotation] Allow parameter pack expansions in annotate attribute

[Aaron Ballman via Phabricator via cfe-commits]

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<void()> 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<typename...Ts> void variadic() {
+template <int... Is> 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