erichkeane added a comment.
I'm down to 1 little thing in how we handle the attribute itself, and I think
@aaron.ballman understands the parsing section better than I do...
================
Comment at: clang/lib/Parse/ParseDecl.cpp:386
ArgsVector ArgExprs;
if (Tok.is(tok::identifier)) {
// If this attribute wants an 'identifier' argument, make it so.
----------------
So does this mean that our pack-parsing code is allowing identifiers? I really
expected this patch to enforce via the clang-attr-emitter that ONLY the 'expr'
types were allowed. So you'd have:
if (attributeAcceptsExprPack(AttrName)) {
... <where this ONLY accepts an expression pack>
} else if (Tok.is(tok::identifier)) {
...
}...
================
Comment at: clang/lib/Sema/SemaDeclAttr.cpp:4179
+ MutableArrayRef<Expr *> AllArgs) {
+ assert(AllArgs.size());
+
----------------
Does this work with a partial specialization? I'd like to see some tests that
ensure we work in that case (both where the partial specialization sets the
string literal correctly, and where it doesnt).
================
Comment at: clang/lib/Sema/SemaDeclAttr.cpp:4202
+ // If the first argument is value dependent we delay setting the arguments.
+ if (AllArgs.size() && AllArgs[0]->isValueDependent()) {
+ auto *Attr = AnnotateAttr::CreateWithDelayedArgs(
----------------
if AllArgs.size() == 0, this is an error case. Annotate requires at least 1
parameter.
================
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);
----------------
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.
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
