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

Thu Dec 2 02:35:56 PST 2021

steffenlarsen 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:
> 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.

================
Comment at: clang/lib/Parse/ParseDecl.cpp:447
+          }
+          ArgExpr = Actions.ActOnPackExpansion(ArgExpr.get(), ConsumeToken());
+        }
----------------
erichkeane wrote:
> Do you have a test for something that isn't a pack followed by an ellipsis?  What is the behavior there?   I'm also concerned that there doesn't seem to be anything here that handles complex-pack expansions (like folds), can you test that as well?
> 
> Also, why is this 'if' not up on line 429 (that is, outside of this 'else'?).  I believe ParseAssignmentExpression is going to muck-up the tokens, so I'm not particularly sure what is going to happen here?
> Do you have a test for something that isn't a pack followed by an ellipsis? What is the behavior there? I'm also concerned that there doesn't seem to be anything here that handles complex-pack expansions (like folds), can you test that as well?

Good question! I would expect it to fail in kind during template instantiation, or earlier if it is not an expression, but I will test it out once we converge on a new solution.

> Also, why is this 'if' not up on line 429 (that is, outside of this 'else'?). I believe ParseAssignmentExpression is going to muck-up the tokens, so I'm not particularly sure what is going to happen here?

I originally tried having the `ActOnPackExpansion` earlier, but it did indeed cause trouble. However, if my understanding of the previous two branches is correct, they are:

  # Type argument. This could be consuming the ellipsis as well, though I suspect it needs another path given that it does not seem to produce any expressions. Note however that it currently seems to stop argument parsing after reading the first type (confirmed by the FIXME) so parameter pack logic for this branch would probably be more suitable once it actually allows attributes to take multiple types.
  # Identifier argument. Correct me if I am wrong, but I don't think this can be populated by a parameter pack.

I could add the diagnostic to these if ellipsis is found, but if my assumptions are correct it doesn't make much sense to expand these branches beyond that.

================
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}}
----------------
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.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D114439