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

[Steffen Larsen via Phabricator via cfe-commits]

steffenlarsen added inline comments.


================
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...)]] ();
 }
----------------
erichkeane wrote:
> 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.
> what about:
> void foz [[clang::annotate("D", Is)]] ();
>
> I would expect that to error.

So would I, but it seems that the parser and annotate is more than happy to consume this example, even in the current state of Clang: https://godbolt.org/z/rx64EKeeE
I am not sure as to why, but seeing as it is a preexisting bug I don't know if it makes sense to include it in the scope of this patch.

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

They seem to work as expected. I have added these cases.


================
Comment at: clang/utils/TableGen/ClangAttrEmitter.cpp:1166
 
   class VariadicExprArgument : public VariadicArgument {
+    bool AllowPack = false;
----------------
erichkeane wrote:
> The rule of 'only the last argument is allowed to support a pack' should be in the attribute emitter.
> The rule of 'only the last argument is allowed to support a pack' should be in the attribute emitter.

I have added some assertions around the area where attributes are generate, as that carries more surrounding information. Is that approximately what you had in mind?


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

https://reviews.llvm.org/D114439