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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
