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

[Erich Keane via Phabricator via cfe-commits]

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