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

[Steffen Larsen via Phabricator via cfe-commits]

steffenlarsen added inline comments.


================
Comment at: clang/include/clang/Basic/Attr.td:545
+  // Set to true if this attribute accepts parameter pack expansion expressions.
+  bit AcceptsExprPack = 0;
   // Lists language options, one of which is required to be true for the
----------------
aaron.ballman wrote:
> You should probably add a release note about this new feature.
I have added a release note about this under "Internal API Changes". Let me know if you think the wording needs changes.


================
Comment at: clang/include/clang/Basic/Attr.td:789
   let PragmaAttributeSupport = 1;
+  let AcceptsExprPack = 1;
   let Documentation = [Undocumented];
----------------
aaron.ballman wrote:
> You should definitely add a release note about this new behavior for annotate.
I have added a release note about this under "Attribute Changes in Clang". As with the other note, let me know if wording needs changes.


================
Comment at: clang/lib/Sema/SemaDeclAttr.cpp:8153
+
+  bool AttrHasVariadicArg = AL.hasVariadicArg();
+  unsigned AttrNumArgs = AL.getNumArgMembers();
----------------
erichkeane wrote:
> steffenlarsen wrote:
> > erichkeane wrote:
> > > This still doesn't work if the VariadicExprArgument isn't last, right?  Do we ensure that is the case in clang-attr-emitter?
> > Good point. I don't think there's a check as there are select few attributes that use multiple variadic (`OMPDeclareSimdDecl` and `OMPDeclareVariant` are the only ones, I think.)
> > 
> > Since I don't think it's safe to check for all, should I make a check similar to the one for type/identifier arguments in attributes marked `AcceptsExprPack`? Would that suffice?
> I'm fine rejecting a case that has anything besides expression-arguments(and ones create-able from expression arguments) and limited-to-only-1-must-be-last variadic-expr-list in ClangAttrEmitter.
> 
> I believe we discussed that at one point, but I didn't see it here.
I have added a check to ClangAttrEmitter next to the check ensuring that the relevant attributes don't have identifier or type arguments.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D114439/new/

https://reviews.llvm.org/D114439