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