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

[Steffen Larsen via Phabricator via cfe-commits]

steffenlarsen marked 2 inline comments as done.
steffenlarsen 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:
> aaron.ballman wrote:
> > Coding style nits.
> We don't typically use top-level `const`, so that should go as well.
Completely missed that. My bad!


================
Comment at: clang/lib/Parse/ParseExpr.cpp:3374-3375
 
+    if (EarlyTypoCorrection)
+      Expr = Actions.CorrectDelayedTyposInExpr(Expr);
+
----------------
aaron.ballman wrote:
> 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.)
11 tests fail if we don't make it conditional. The most telling one is probably `SemaCXX/typo-correction-delayed.cpp` which will have more errors if we do typo correction early, so it seems that the delay is intentional. 


================
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}}
----------------
aaron.ballman wrote:
> You can back out the whitespace changes for an ever-so-slightly smaller patch. :-D
Absolutely. Clang-format was very insistent, but since it isn't added by this patch I shouldn't worry. 😄 


================
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)]] ();
+}
----------------
aaron.ballman wrote:
> Shouldn't these cases also give the `// expected-error {{pack expansion in 'annotate' may only be applied to the last argument}}` diagnostic?
These should preferably pass. They are part of the "last" argument as `annotate` has one non-variadic argument and one variadic argument. I agree though that the error message is a little vague. Originally the error did specify that it had to be "argument %1 or later" to try and convey this intent, but maybe you have an idea for a clearer error message?


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

https://reviews.llvm.org/D114439