[PATCH] D114439: [Annotation] Allow parameter pack expansions and initializer lists in annotate attribute

aaron.ballman closed this revision. aaron.ballman added a comment. I've commit on your behalf in ead1690d31f815c00fdd2bc23db4766191bbeabc , thank you! CHANGES SINCE LAST ACTION https://reviews.llvm.org/D114439/new/ https:/

[PATCH] D114439: [Annotation] Allow parameter pack expansions and initializer lists in annotate attribute

aaron.ballman accepted this revision. aaron.ballman added a comment. This revision is now accepted and ready to land. LGTM! Thank you for all the good discussion and work on this! I'll commit on your behalf once the precommit CI pipeline comes back green. CHANGES SINCE LAST ACTION https://rev

[PATCH] D114439: [Annotation] Allow parameter pack expansions and initializer lists in annotate attribute

steffenlarsen added a comment. Thanks, @aaron.ballman ! I have applied the new suggestions. > I presume you don't have commit privileges @steffenlarsen? If that's > accurate, what name and email address would you like me to use for patch > attribution after you've fixed the last few bits? That

[PATCH] D114439: [Annotation] Allow parameter pack expansions and initializer lists in annotate attribute

steffenlarsen updated this revision to Diff 406868. steffenlarsen added a comment. Recent changes: - Renamed `isArgMemberExprHolder` to `isParamExpr` and added the comment suggested by @aaron.ballman . - Made `checkASCIIStringLiteralExpr` an overload of `checkStringLiteralArgumentAttr`. CHANG

[PATCH] D114439: [Annotation] Allow parameter pack expansions and initializer lists in annotate attribute

aaron.ballman added a comment. I still have some naming issues that need to be corrected, but functionally I think this is good to go. I presume you don't have commit privileges @steffenlarsen? If that's accurate, what name and email address would you like me to use for patch attribution after

[PATCH] D114439: [Annotation] Allow parameter pack expansions and initializer lists in annotate attribute

steffenlarsen added inline comments. Comment at: clang/include/clang/Sema/ParsedAttr.h:113 } + // Check if argument at index I is an expression argument + virtual bool isExprArg(size_t N) const { return false; } aaron.ballman wrote: > I don't think this API

[PATCH] D114439: [Annotation] Allow parameter pack expansions and initializer lists in annotate attribute

steffenlarsen updated this revision to Diff 406468. steffenlarsen added a comment. Recent changes: - Replaces `isExprArg` with `isArgMemberExprHolder` which returns true if the "argument member" can hold one or more expressions. - Renames `checkStringLiteralExpr` to `checkASCIIStringLiteralExpr`

[PATCH] D114439: [Annotation] Allow parameter pack expansions and initializer lists in annotate attribute

aaron.ballman added inline comments. Comment at: clang/include/clang/Sema/ParsedAttr.h:52-57 + /// The number of non-fake arguments specified in the attribute definition. + unsigned NumArgMembers : 4; /// True if the parsing does not match the semantic content. unsigned H

[PATCH] D114439: [Annotation] Allow parameter pack expansions and initializer lists in annotate attribute

steffenlarsen updated this revision to Diff 406373. steffenlarsen added a comment. Added a check to prevent duplicate "empty" constructors, i.e. if there are only optional arguments such as a single variadic argument. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D114439/new/ https://re

[PATCH] D114439: [Annotation] Allow parameter pack expansions and initializer lists in annotate attribute

steffenlarsen updated this revision to Diff 406094. steffenlarsen added a comment. Added missing `isVariadicExprArgument` function. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D114439/new/ https://reviews.llvm.org/D114439 Files: clang/docs/ReleaseNotes.rst clang/include/clang/Basi

[PATCH] D114439: [Annotation] Allow parameter pack expansions and initializer lists in annotate attribute

steffenlarsen added inline comments. 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

[PATCH] D114439: [Annotation] Allow parameter pack expansions and initializer lists in annotate attribute

steffenlarsen updated this revision to Diff 405963. steffenlarsen added a comment. Added check restricting variadic expression arguments to the last argument of attributes set to support parameter packs. Added release notes. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D114439/new/ htt

[PATCH] D114439: [Annotation] Allow parameter pack expansions and initializer lists in annotate attribute

erichkeane added inline comments. Comment at: clang/lib/Sema/SemaDeclAttr.cpp:8153 + + bool AttrHasVariadicArg = AL.hasVariadicArg(); + unsigned AttrNumArgs = AL.getNumArgMembers(); steffenlarsen wrote: > erichkeane wrote: > > This still doesn't work if the Var

[PATCH] D114439: [Annotation] Allow parameter pack expansions and initializer lists in annotate attribute

steffenlarsen marked 2 inline comments as done. steffenlarsen added inline comments. Comment at: clang/lib/Sema/SemaDeclAttr.cpp:8153 + + bool AttrHasVariadicArg = AL.hasVariadicArg(); + unsigned AttrNumArgs = AL.getNumArgMembers(); erichkeane wrote: > This sti

[PATCH] D114439: [Annotation] Allow parameter pack expansions and initializer lists in annotate attribute

erichkeane added a comment. 1 question, otherwise happy. Comment at: clang/lib/Sema/SemaDeclAttr.cpp:8153 + + bool AttrHasVariadicArg = AL.hasVariadicArg(); + unsigned AttrNumArgs = AL.getNumArgMembers(); This still doesn't work if the VariadicExprArgument is

[PATCH] D114439: [Annotation] Allow parameter pack expansions and initializer lists in annotate attribute

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

[PATCH] D114439: [Annotation] Allow parameter pack expansions and initializer lists in annotate attribute

steffenlarsen updated this revision to Diff 405820. steffenlarsen added a comment. Adjusted for comments and introduced common handling for creating attributes with delayed arguments. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D114439/new/ https://reviews.llvm.org/D114439 Files: c

[PATCH] D114439: [Annotation] Allow parameter pack expansions and initializer lists in annotate attribute

aaron.ballman added inline comments. 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); ---

[PATCH] D114439: [Annotation] Allow parameter pack expansions and initializer lists in annotate attribute

erichkeane added inline comments. 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); a

[PATCH] D114439: [Annotation] Allow parameter pack expansions and initializer lists in annotate attribute

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 Accepts

[PATCH] D114439: [Annotation] Allow parameter pack expansions and initializer lists in annotate attribute

erichkeane added a comment. I think I'm happy, though there are the opens for Aaron (deciding whether the 'create as delayed' should happen without entry into 'handleAnnotateAttr', and whether the 'parsing' looks correct, despite not being the way we discussed), so you'll need to wait on him.

[PATCH] D114439: [Annotation] Allow parameter pack expansions and initializer lists in annotate attribute

steffenlarsen marked 3 inline comments as done. steffenlarsen added inline comments. Comment at: clang/lib/Sema/SemaDeclAttr.cpp:4179 + MutableArrayRef AllArgs) { + assert(AllArgs.size()); + erichkeane wrote: > Does this work with a

[PATCH] D114439: [Annotation] Allow parameter pack expansions and initializer lists in annotate attribute

steffenlarsen updated this revision to Diff 404889. steffenlarsen added a comment. Added tests for redeclarations and template specialization using `clang::annoate` with packs. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D114439/new/ https://reviews.llvm.org/D114439 Files: clang/in

[PATCH] D114439: [Annotation] Allow parameter pack expansions and initializer lists in annotate attribute

erichkeane added inline comments. 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(

[PATCH] D114439: [Annotation] Allow parameter pack expansions and initializer lists in annotate attribute

steffenlarsen added inline comments. 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( -

[PATCH] D114439: [Annotation] Allow parameter pack expansions and initializer lists in annotate attribute

erichkeane added inline comments. 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(

[PATCH] D114439: [Annotation] Allow parameter pack expansions and initializer lists in annotate attribute

steffenlarsen added inline comments. 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( -

[PATCH] D114439: [Annotation] Allow parameter pack expansions and initializer lists in annotate attribute

steffenlarsen updated this revision to Diff 404548. steffenlarsen added a comment. Adds check and diagnostic for no arguments in annotate attribute. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D114439/new/ https://reviews.llvm.org/D114439 Files: clang/include/clang/Basic/Attr.td c

[PATCH] D114439: [Annotation] Allow parameter pack expansions and initializer lists in annotate attribute

steffenlarsen added inline comments. 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( -

[PATCH] D114439: [Annotation] Allow parameter pack expansions and initializer lists in annotate attribute

steffenlarsen updated this revision to Diff 404526. steffenlarsen added a comment. Removed unnecessary test for arguments. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D114439/new/ https://reviews.llvm.org/D114439 Files: clang/include/clang/Basic/Attr.td clang/include/clang/Basic/D

[PATCH] D114439: [Annotation] Allow parameter pack expansions and initializer lists in annotate attribute

steffenlarsen added inline comments. 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. steffenlarsen wrote: > erichkeane wrote: > > So does

[PATCH] D114439: [Annotation] Allow parameter pack expansions and initializer lists in annotate attribute

steffenlarsen marked 2 inline comments as done. steffenlarsen added inline comments. 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. eric

[PATCH] D114439: [Annotation] Allow parameter pack expansions and initializer lists in annotate attribute

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)) {

[PATCH] D114439: [Annotation] Allow parameter pack expansions and initializer lists in annotate attribute

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, no

[PATCH] D114439: [Annotation] Allow parameter pack expansions and initializer lists in annotate attribute

steffenlarsen updated this revision to Diff 404440. steffenlarsen edited the summary of this revision. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D114439/new/ https://reviews.llvm.org/D114439 Files: clang/include/clang/Basic/Attr.td clang/include/clang/Basic/DiagnosticParseKinds.td

[PATCH] D114439: [Annotation] Allow parameter pack expansions and initializer lists in annotate attribute

aaron.ballman added a comment. I agree with the comments left by @erichkeane regarding some still needed tweaks to the approach. Comment at: clang/include/clang/Basic/Attr.td:208 class VariadicUnsignedArgument : Argument; -class VariadicExprArgument : Argument; +class Variadi

[PATCH] D114439: [Annotation] Allow parameter pack expansions and initializer lists in annotate attribute

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 f

[PATCH] D114439: [Annotation] Allow parameter pack expansions and initializer lists in annotate attribute

steffenlarsen added a comment. Upon offline sync with @aaron.ballman and @erichkeane I have changed the strategy of this patch to lift the restriction of pack expansion not spanning argument boundaries. This is implemented in `clang::annotate` by delaying arguments to after template-instantiat

[PATCH] D114439: [Annotation] Allow parameter pack expansions and initializer lists in annotate attribute

steffenlarsen updated this revision to Diff 403578. steffenlarsen retitled this revision from "[Annotation] Allow parameter pack expansions in annotate attribute" to "[Annotation] Allow parameter pack expansions and initializer lists in annotate attribute". steffenlarsen edited the summary of thi