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

[Erich Keane via Phabricator via cfe-commits]

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