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

[Steffen Larsen via Phabricator via cfe-commits]

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