erichkeane added a comment.
Took a look, it doesn't seem that this reflects the discussion we had yesterday.
================
Comment at: clang/include/clang/AST/Attr.h:51
unsigned Implicit : 1;
+ unsigned ArgsDelayed : 1;
// FIXME: These are properties of the attribute kind, not state for this
----------------
What is this supposed to be for? We should be able to tell if we weren't able
to instantiate the expressions based on whether the expr-list has stuff in it,
and if it is dependent.
================
Comment at: clang/include/clang/Basic/Attr.td:208
class VariadicUnsignedArgument<string name> : Argument<name, 1>;
-class VariadicExprArgument<string name> : Argument<name, 1>;
+class VariadicExprArgument<string name, bit fake = 0> : Argument<name, 1,
fake>;
class VariadicStringArgument<string name> : Argument<name, 1>;
----------------
I'm pretty against this 'fake', we should be putting this into the TableGen for
every attribute that has AcceptsExprPack.
================
Comment at: clang/include/clang/Basic/Attr.td:777
+ VariadicExprArgument<"Args">,
+ VariadicExprArgument<"DelayedArgs", /*fake=*/1>];
// Ensure that the annotate attribute can be used with
----------------
See above.
================
Comment at: clang/lib/Parse/ParseDecl.cpp:460
+ CommaLocsTy CommaLocs;
+ ExprVector ParsedExprs;
+ if (ParseExpressionList(ParsedExprs, CommaLocs,
----------------
So this all doesn't seem right either. The algorithm here is essentially:
if (AcceptsArgPack) {
ParseExpressionList(); // plus error handling
// Loop through args to see what matches correctly, if the
non-VariadicExprList arguments are non-dependent, fill those in, and create as
normal, else keep in the materialized collection.
}
================
Comment at: clang/lib/Sema/SemaDeclAttr.cpp:4189
+void Sema::AddAnnotationAttrWithDelayedArgs(
+ Decl *D, const AttributeCommonInfo &CI,
----------------
I think this behavior (adding an attribute with a dependent expression list)
should be happening automatically.
In reality, whatever is doing the parsing or potentially-partial-instantiation
should be checking the 'StringLiteral' value as a part of how it would without
this being dependent.
That is: In the annotate case, the ONLY argument that we actually care about
being dependent or not is the 'first' one. When we do the
parsing/instantiation, we need to detect if the non-variadic parameters can be
'filled', then we can put the rest into the VariadicExpr list.
================
Comment at: clang/lib/Sema/SemaTemplateInstantiateDecl.cpp:196
+ SmallVector<Expr *, 4> InstantiatedArgs;
+ if (S.SubstExprs(ArrayRef<Expr *>{Attr->delayedArgs_begin(),
+ Attr->delayedArgs_end()},
----------------
So I don't think this is the right approach (teaching SemaDeclAttr about the
delayed args). After substitution here, we should be using these to fill in
the 'normal' arguments, then just passing these to the normal
'handleAnnotateAttr'.
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
