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

[Steffen Larsen via Phabricator via cfe-commits]

steffenlarsen marked 3 inline comments as done.
steffenlarsen added inline comments.


================
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:
> 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).
I see what you mean. I have applied your wording instead.


================
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);
----------------
aaron.ballman wrote:
> erichkeane wrote:
> > aaron.ballman wrote:
> > > 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)?
> > Ah, yes, thats what I mean.  The question for ME is whether there should be a generic "this supports variadic pack, so check to see if all the named non-expr arguments are fill-in-able.  If not, do the 'delayed' version.
> > 
> > This would mean that HandleAnnotateAttr NEVER sees the "CreateWithDelayedArgs" case.
> Thanks for clarifying -- yes, I think that would be preferable if it works out in a clean, generic way. I'd be fine with tablegen emitting something else (if necessary) to help generalize it.
`handleAnnotateAttr` is now oblivious to the concept of "delayed arguments". Instead tablegen generates a common handle function (`handleAttrWithDelayedArgs`) which will, based on the given `ParsedAttr` that supports delayed arguments, create and add the corresponding attribute with delayed arguments by calling the corresponding `CreateWithDelayedArgs`. The need for delaying arguments is decided as described in `MustDelayAttributeArguments`.

Is this approximately what you were thinking?


================
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;
+  }
----------------
aaron.ballman wrote:
> 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");`
It does go through `checkCommonAttributeFeatures` but (as of the recent changes) it will skip size-checks if arguments are delayed as a pack expansion could potentially populate the seemingly missing expressions after template instantiation for some attribute.
For annotate we could also have a pack as the only expression, which would then evaluate to an empty list of expressions. Since this path is also taken by `instantiateDependentAnnotationAttr` if there are delayed args. In reality it is only really needed after template instantiations, given as you said `checkCommonAttributeFeatures` will do the checking in the other case, but I personally think it is cleaner to have it here. If you disagree I will move it into `instantiateDependentAnnotationAttr` instead and add the assert.


================
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
----------------
aaron.ballman wrote:
> 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).
I have done this. It is a little more verbose, but I agree with the full-picture sentiment.


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

https://reviews.llvm.org/D114439