[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 think this is heading in the right direction! I've got a few more comments and questions though.



================
Comment at: clang/include/clang/Basic/Attr.td:545
+  // Set to true if this attribute accepts parameter pack expansion expressions.
+  bit AcceptsExprPack = 0;
   // Lists language options, one of which is required to be true for the
----------------
You should probably add a release note about this new feature.


================
Comment at: clang/include/clang/Basic/Attr.td:789
   let PragmaAttributeSupport = 1;
+  let AcceptsExprPack = 1;
   let Documentation = [Undocumented];
----------------
You should definitely add a release note about this new behavior for annotate.


================
Comment at: clang/include/clang/Basic/Attr.td:790
+  let AcceptsExprPack = 1;
   let Documentation = [Undocumented];
 }
----------------
Le sigh. We should update the documentation for this new feature, but we have no documentation for the feature *to* update. (This is not your problem to solve in this patch.)


================
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.
----------------
steffenlarsen wrote:
> 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?
Maybe we're thinking about identifier lists differently. We only have two attributes that use those (`cpu_specific` and `cpu_dispatch`) and in both cases (and all cases I would expect), what's being received is effectively a list of enumerators (not enumerators in the C or C++ sense) that could not be mixed with expressions. Expressions would go through sema and do all the usual lookup work to turn them into a value, but these are not real objects and so the lookup would fail for them. e.g., we're not going to be able to support something like: `[[clang::cpu_specific(generic, pentium, Ts..., atom)]]`. So I don't think there's anything here to be fixed (I prefer my comment formulation as that makes it more clear).


================
Comment at: clang/lib/Sema/SemaDeclAttr.cpp:4179-4182
+  if (AllArgs.size() < 1) {
+    Diag(CI.getLoc(), diag::err_attribute_too_few_arguments) << CI << 1;
+    return;
+  }
----------------
Please double-check that the call to `checkCommonAttributeFeatures()` from `ProcessDeclAttribute()` doesn't already handle this for you. If it does, then I think this can be replaced with `assert(!AllArgs.empty() && "expected at least one argument");`


================
Comment at: clang/lib/Sema/SemaDeclAttr.cpp:4203
+  if (AllArgs.size() && AllArgs[0]->isValueDependent()) {
+    auto *Attr = AnnotateAttr::CreateWithDelayedArgs(
+        S.getASTContext(), AllArgs.data(), AllArgs.size(), AL);
----------------
erichkeane wrote:
> I would like @aaron.ballman to comment on this, but I think we probably want this case to be covered in the top of `HandleDeclAttr`, which would mean in the 'not all values filled' case, we skip the 'handleAnnotateAttr'.  
> 
> WDYT Aaron?  The downside is that this function couldn't check a 'partially filled in' attribute, but it would make us that much closer to this flag being a very simple thing to opt into.
Do you mean `ProcessDeclAttribute()`? I don't think we should have attribute-specific logic in there, but are you thinking of something more general than that (I'm not seeing how the suggestion makes the flag easier to opt into)?


================
Comment at: clang/test/SemaTemplate/attributes.cpp:231-240
+// CHECK:      FunctionTemplateDecl {{.*}} RedeclaredAnnotatedFunc
+// CHECK:        AnnotateAttr {{.*}} Inherited "ANNOTATE_FAR"
+// CHECK:        AnnotateAttr {{.*}} Inherited "ANNOTATE_FIZ"
+// CHECK:          ConstantExpr {{.*}} 'int'
+// CHECK-NEXT:       value: Int 4
+// CHECK:          ConstantExpr {{.*}} 'int'
+// CHECK-NEXT:       value: Int 5
----------------
I was thrown by the CHECK and CHECK-NEXT mixtures here because I couldn't see that the inherited attributes were or were not also getting the expanded pack values. You should make sure to include all of the lines with CHECK-NEXT so we see a full picture of the AST dump (this file is not super consistent about it).


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

https://reviews.llvm.org/D114439