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

[Steffen Larsen via Phabricator via cfe-commits]

steffenlarsen marked 7 inline comments as done and an inline comment as not done.
steffenlarsen added inline comments.


================
Comment at: clang/include/clang/AST/Attr.h:51
   unsigned Implicit : 1;
+  unsigned ArgsDelayed : 1;
   // FIXME: These are properties of the attribute kind, not state for this
----------------
erichkeane wrote:
> What is this supposed to be for?  We should be able to tell if we weren't able to instantiate the expressions based on whether the expr-list has stuff in it, and if it is dependent.
You're right, it is redundant. I have removed it.


================
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>;
----------------
aaron.ballman wrote:
> 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.
I see. I have changed it to automatically generate a variadic expression "argument" named `DelayedArgs` if an attribute has `AcceptsExprPack` set. 


================
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.
----------------
aaron.ballman wrote:
> (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.
I think it makes sense to have it be a FIXME because in theory it could be possible to have expression parameter packs expanded in an identifier list as it accepts expressions. I have reworded it slightly. Do you think this is better?


================
Comment at: clang/lib/Parse/ParseDecl.cpp:460
+      CommaLocsTy CommaLocs;
+      ExprVector ParsedExprs;
+      if (ParseExpressionList(ParsedExprs, CommaLocs,
----------------
erichkeane wrote:
> So this all doesn't seem right either.  The algorithm here is essentially:
> 
> if (AcceptsArgPack) {
>   ParseExpressionList(); // plus error handling
> 
> //  Loop through args to see what matches correctly, if the non-VariadicExprList arguments are non-dependent, fill those in, and create as normal, else keep in the materialized collection.
> }
I am not convinced this is the right place for that. These changes are simply to allow the additional parsing of packs and similar. It does not care much for the different arguments of the attribute, beyond whether or not it accepts types or identifiers as well (see the other cases) so having it make decisions about how the attributes should populate their arguments seems out of line with the current structure.


================
Comment at: clang/lib/Sema/SemaDeclAttr.cpp:4189
 
+void Sema::AddAnnotationAttrWithDelayedArgs(
+    Decl *D, const AttributeCommonInfo &CI,
----------------
erichkeane wrote:
> I think this behavior (adding an attribute with a dependent expression list) should be happening automatically.
> 
> In reality, whatever is doing the parsing or potentially-partial-instantiation should be checking the 'StringLiteral' value as a part of how it would without this being dependent.
> 
> That is: In the annotate case, the ONLY argument that we actually care about being dependent or not is the 'first' one.  When we do the parsing/instantiation, we need to detect if the non-variadic parameters can be 'filled', then we can put the rest into the VariadicExpr list.
I have changed it so it will only delay the arguments if the the first argument is dependent. Is this in line with what you were thinking? Since we still need to create the attribute I am not sure how we would do this automatically.


================
Comment at: clang/lib/Sema/SemaTemplateInstantiateDecl.cpp:196
+    SmallVector<Expr *, 4> InstantiatedArgs;
+    if (S.SubstExprs(ArrayRef<Expr *>{Attr->delayedArgs_begin(),
+                                      Attr->delayedArgs_end()},
----------------
erichkeane wrote:
> So I don't think this is the right approach (teaching SemaDeclAttr about the delayed args).  After substitution here, we should be using these to fill in the 'normal' arguments, then just passing these to the normal 'handleAnnotateAttr'.
With the new structure I have simplified this part a little. SemaDeclAttr still knows about the delayed args but solely to create the attribute, while the logic for creating the attribute with the "normal" arguments has been separated and made part of Sema so it is reachable from this function.


================
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)]] ();
+}
----------------
aaron.ballman wrote:
> 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.
Good point. I will add these tests ASAP.


================
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);
+                         })) &&
----------------
aaron.ballman wrote:
> 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).
I did not know! Thanks. It has been changed. 😄 


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

https://reviews.llvm.org/D114439