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

[Steffen Larsen via Phabricator via cfe-commits]

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 is needed. `isArgExpr()` already exists on the interface and I'm not certain how this differs. (If this API is needed, we need a far better name for it because I'd have no idea how to distinguish `isExprArg()` and `isArgExpr()`.)
There is an important distinction between the two, specifically that `isArgExpr` checks if the parsed argument at the index is an expression, while `isExprArg` checks if the specified argument in the attribute is an "ExprArgument", i.e. one checks post-parsing the other reflects on the attribute's definition.

That said, I agree that with that naming there's little distinction between the two. I have therefore removed `isExprArg` and replaced it with `isArgMemberExprHolder` which is true if the "argument member" (referring to the argument defined in Attr.td) is able to hold an expression (i.e. it is either a `ExprArgument` or a `VaridicExprArgument`.) Functionally there is little change to it, as the only place we use the check is after checking if the expression is variadic, but it fit better with the actual intent of the check.


================
Comment at: clang/include/clang/Sema/Sema.h:4383-4385
+  bool checkStringLiteralExpr(const AttributeCommonInfo &CI, const Expr *E,
+                              StringRef &Str,
+                              SourceLocation *ArgLocation = nullptr);
----------------
aaron.ballman wrote:
> This name makes it sound like far more general of an API than it is. This function is specific to checking string literal arguments for an attribute. But then why does `checkStringLiteralArgumentAttr()` not suffice? It does the same thing, but with more bells and whistles.
The problem is that `checkStringLiteralArgumentAttr()` requires the arguments to be extracted from the parsed attribute, which we do not have after template instantiation, so the split was for generality. However, as you mentioned in another comment, the generality is not that needed anymore so `handleAnnotateAttr` has been reverted to the old version.

That said, this is still needed for doing post-initalization check if arguments have been delayed, so I have renamed it to `checkASCIIStringLiteralExpr` which hopefully conveys the intent a little better?


================
Comment at: clang/lib/Sema/SemaDeclAttr.cpp:4185
   StringRef Str;
-  if (!S.checkStringLiteralArgumentAttr(AL, 0, Str))
+  if (!checkStringLiteralExpr(CI, AllArgs[0], Str))
     return;
----------------
aaron.ballman wrote:
> This is a slight loss of functionality. We used to recommend identifiers be converted to string literals under the old interface. I don't think this refactoring is necessary any longer, right?
You're absolutely right. There is actually no reason for this refactoring anymore, except for maybe some minor common code between this and delayed argument instantiation, but it is so little now that if we want to keep the hint (which I agree we should) then there's little reason not to revert the changes to `handleAnnotateAttr`. So that's what I've done! Similarly, `handleAnnotateAttr` is no longer needed so it has been removed and the excess logic has been moved to `instantiateDependentAnnotationAttr`. Thanks for pointing this out!


================
Comment at: clang/utils/TableGen/ClangAttrEmitter.cpp:2312
+
+    // All these spellings take are parsed unevaluated.
+    forEachUniqueSpelling(Attr, [&](const FlattenedSpelling &S) {
----------------
aaron.ballman wrote:
> The comment doesn't seem grammatical and may need some expounding.
I don't think a comment makes much sense here anyway, so I've removed it. This seems like fairly common practice for these kind of functions. If you disagree I can reintroduce it and expand on it.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D114439/new/

https://reviews.llvm.org/D114439