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

[Erich Keane via Phabricator via cfe-commits]

erichkeane added a comment.

Took a look, it doesn't seem that this reflects the discussion we had yesterday.



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


================
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>;
----------------
I'm pretty against this 'fake', we should be putting this into the TableGen for every attribute that has AcceptsExprPack.


================
Comment at: clang/include/clang/Basic/Attr.td:777
+              VariadicExprArgument<"Args">,
+              VariadicExprArgument<"DelayedArgs", /*fake=*/1>];
   // Ensure that the annotate attribute can be used with
----------------
See above.




================
Comment at: clang/lib/Parse/ParseDecl.cpp:460
+      CommaLocsTy CommaLocs;
+      ExprVector ParsedExprs;
+      if (ParseExpressionList(ParsedExprs, CommaLocs,
----------------
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.
}


================
Comment at: clang/lib/Sema/SemaDeclAttr.cpp:4189
 
+void Sema::AddAnnotationAttrWithDelayedArgs(
+    Decl *D, const AttributeCommonInfo &CI,
----------------
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.


================
Comment at: clang/lib/Sema/SemaTemplateInstantiateDecl.cpp:196
+    SmallVector<Expr *, 4> InstantiatedArgs;
+    if (S.SubstExprs(ArrayRef<Expr *>{Attr->delayedArgs_begin(),
+                                      Attr->delayedArgs_end()},
----------------
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'.


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

https://reviews.llvm.org/D114439