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

_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Reply via email to