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

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.


================
Comment at: clang/lib/Parse/ParseDecl.cpp:497
+        // Pack expansion is only allowed in the variadic argument at the end.
+        const size_t attrNumArgs = attributeNumberOfArguments(*AttrName);
+        if (ArgExprs.size() + I + 1 < attrNumArgs) {
----------------
aaron.ballman wrote:
> Coding style nits.
We don't typically use top-level `const`, so that should go as well.


================
Comment at: clang/lib/Parse/ParseExpr.cpp:3374-3375
 
+    if (EarlyTypoCorrection)
+      Expr = Actions.CorrectDelayedTyposInExpr(Expr);
+
----------------
steffenlarsen wrote:
> aaron.ballman wrote:
> > steffenlarsen wrote:
> > > aaron.ballman wrote:
> > > > Not that I think this is a bad change, but it seems unrelated to the patch. Can you explain why you made the change?
> > > Admittedly I am not sure why it is needed, but in the previous version of the parsing for attribute parameters it does the typo-correction immediately after parsing the expression and if this isn't added a handful of tests fail.
> > Ah, thanks for the info! So this is basically doing the same work as what's done on line 3410 in the late failure case -- I wonder if this should actually be tied to the `FailImmediatelyOnInvalidExpr` parameter instead of having its own parameter?
> They could be unified in the current version, but I don't think there is an actual relation between them.
Do we fail tests if this isn't conditionalized at all (we always do the early typo correction)? (I'd like to avoid adding the new parameter because I'm not certain how a caller would determine whether they do or do not want that behavior. If we need the parameter, then so be it, but it seems like this is something generally useful.)


================
Comment at: clang/test/Parser/cxx0x-attributes.cpp:262
 
-template<typename...Ts> void variadic() {
+template <typename... Ts> void variadic() {
   void bar [[noreturn...]] (); // expected-error {{attribute 'noreturn' cannot be used as an attribute pack}}
----------------
You can back out the whitespace changes for an ever-so-slightly smaller patch. :-D


================
Comment at: clang/test/Parser/cxx0x-attributes.cpp:276-277
+  void foz [[clang::annotate("E", 1, 2, 3, Is...)]] ();
+  void fiz [[clang::annotate("F", Is..., 1, 2, 3)]] ();
+  void fir [[clang::annotate("G", 1, Is..., 2, 3)]] ();
+}
----------------
Shouldn't these cases also give the `// expected-error {{pack expansion in 'annotate' may only be applied to the last argument}}` diagnostic?


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

https://reviews.llvm.org/D114439