steffenlarsen marked 7 inline comments as done and an inline comment as not
done.
steffenlarsen added inline comments.
================
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
----------------
erichkeane wrote:
> 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.
You're right, it is redundant. I have removed it.
================
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>;
----------------
aaron.ballman wrote:
> erichkeane wrote:
> > I'm pretty against this 'fake', we should be putting this into the TableGen
> > for every attribute that has AcceptsExprPack.
> Yeah, Fake is intended to be used for "internal details" and means that we
> won't pretty print out that data from the AST node. These arguments aren't
> really fake in that way -- they're things the user wrote in their source code.
I see. I have changed it to automatically generate a variadic expression
"argument" named `DelayedArgs` if an attribute has `AcceptsExprPack` set.
================
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:
> (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?
================
Comment at: clang/lib/Parse/ParseDecl.cpp:460
+ CommaLocsTy CommaLocs;
+ ExprVector ParsedExprs;
+ if (ParseExpressionList(ParsedExprs, CommaLocs,
----------------
erichkeane wrote:
> 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.
> }
I am not convinced this is the right place for that. These changes are simply
to allow the additional parsing of packs and similar. It does not care much for
the different arguments of the attribute, beyond whether or not it accepts
types or identifiers as well (see the other cases) so having it make decisions
about how the attributes should populate their arguments seems out of line with
the current structure.
================
Comment at: clang/lib/Sema/SemaDeclAttr.cpp:4189
+void Sema::AddAnnotationAttrWithDelayedArgs(
+ Decl *D, const AttributeCommonInfo &CI,
----------------
erichkeane wrote:
> 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.
I have changed it so it will only delay the arguments if the the first argument
is dependent. Is this in line with what you were thinking? Since we still need
to create the attribute I am not sure how we would do this automatically.
================
Comment at: clang/lib/Sema/SemaTemplateInstantiateDecl.cpp:196
+ SmallVector<Expr *, 4> InstantiatedArgs;
+ if (S.SubstExprs(ArrayRef<Expr *>{Attr->delayedArgs_begin(),
+ Attr->delayedArgs_end()},
----------------
erichkeane wrote:
> 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'.
With the new structure I have simplified this part a little. SemaDeclAttr still
knows about the delayed args but solely to create the attribute, while the
logic for creating the attribute with the "normal" arguments has been separated
and made part of Sema so it is reachable from this function.
================
Comment at: clang/test/Parser/cxx0x-attributes.cpp:276
+ void fiz [[clang::annotate("F", Is..., 1, 2, 3)]] ();
+ void fir [[clang::annotate("G", 1, Is..., 2, 3)]] ();
+}
----------------
aaron.ballman wrote:
> We should also be adding test coverage for things like redeclarations,
> partial specializations, etc to make sure that the behavior there is correct.
> Those would be Sema tests rather than Parser tests, however.
Good point. I will add these tests ASAP.
================
Comment at: clang/utils/TableGen/ClangAttrEmitter.cpp:2349-2357
+ assert((!AttrAcceptsExprPack ||
+ std::none_of(ArgRecords.begin(), ArgRecords.end(),
+ [&](const Record *ArgR) {
+ return isIdentifierArgument(ArgR) ||
+ isVariadicIdentifierArgument(ArgR) ||
+ isTypeArgument(ArgR);
+ })) &&
----------------
aaron.ballman wrote:
> Instead of making this an assert, I think we should use `PrintFatalError()`
> (this is how you effectively emit a diagnostic when compiling a .td file).
I did not know! Thanks. It has been changed. 😄
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
