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

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

I agree with the comments left by @erichkeane regarding some still needed tweaks to the approach.



================
Comment at: clang/include/clang/Basic/Attr.td:208
 class VariadicUnsignedArgument<string name> : Argument<name, 1>;
-class VariadicExprArgument<string name> : Argument<name, 1>;
+class VariadicExprArgument<string name, bit fake = 0> : Argument<name, 1, fake>;
 class VariadicStringArgument<string name> : Argument<name, 1>;
----------------
erichkeane wrote:
> I'm pretty against this 'fake', we should be putting this into the TableGen for every attribute that has AcceptsExprPack.
Yeah, Fake is intended to be used for "internal details" and means that we won't pretty print out that data from the AST node. These arguments aren't really fake in that way -- they're things the user wrote in their source code.


================
Comment at: clang/lib/Parse/ParseDecl.cpp:421-424
+      // Parse variadic identifier arg. This can either consume identifiers or
+      // expressions.
+      // FIXME: Variadic identifier args do not currently support parameter
+      //        packs.
----------------
(Might need to re-flow comments to 80 col.) I don't think this is a FIXME so much as a "this just doesn't work like that" situation.


================
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:
> > > > 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. 
Good to know, thank you for checking! I agree, it sounds like the delay is intentional then. I think it's okay to leave the parameter as you have it, but it is unfortunate that we've made the interface this much more complicated.


================
Comment at: clang/test/Parser/cxx0x-attributes.cpp:276
+  void fiz [[clang::annotate("F", Is..., 1, 2, 3)]] ();
+  void fir [[clang::annotate("G", 1, Is..., 2, 3)]] ();
+}
----------------
We should also be adding test coverage for things like redeclarations, partial specializations, etc to make sure that the behavior there is correct. Those would be Sema tests rather than Parser tests, however.


================
Comment at: clang/utils/TableGen/ClangAttrEmitter.cpp:2349-2357
+    assert((!AttrAcceptsExprPack ||
+            std::none_of(ArgRecords.begin(), ArgRecords.end(),
+                         [&](const Record *ArgR) {
+                           return isIdentifierArgument(ArgR) ||
+                                  isVariadicIdentifierArgument(ArgR) ||
+                                  isTypeArgument(ArgR);
+                         })) &&
----------------
Instead of making this an assert, I think we should use `PrintFatalError()` (this is how you effectively emit a diagnostic when compiling a .td file).


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

https://reviews.llvm.org/D114439