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

Thu Dec 2 08:40:43 PST 2021

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 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 another reason not to allow type packs initially. :-)
> 
> > 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. 
> 
> 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>];
> ```
> 
I agree, BOTH of those should be prohibited.  Any amount of mixing of VariadicExprArgument (or mixing of any of the variadics IMO) need to be illegal.  Otherwise the ambiguous nature of it is untenable.

While I like the possibility of 
`let Args = [ExprArgument<"Arg1">, ExprArgument<"Arg2">];`

ALSO accepting a pack, I think we end up getting a little too 'hacky' with how to store this in the non-instantiated version.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D114439