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
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
