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

[Aaron Ballman via Phabricator via cfe-commits]

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 you've fixed the last few bits?



================
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; }
----------------
steffenlarsen wrote:
> 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.
Thanks for the clarification!

I think a better way to express this is by naming it `isParamExpr()` and adding comments along the lines of:

```
/// Returns true if the specified parameter index for this attribute in Attr.td is an ExprArgument
/// or VariadicExprArgument, or a subclass thereof; returns false otherwise.
```
(Note, the local style here is to use `///` instead of `//`, so I'm matching that as well. You probably need to flow the comments to 80 cols as well.)


================
Comment at: clang/include/clang/Sema/Sema.h:4383-4385
+  bool checkStringLiteralExpr(const AttributeCommonInfo &CI, const Expr *E,
+                              StringRef &Str,
+                              SourceLocation *ArgLocation = nullptr);
----------------
steffenlarsen wrote:
> 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?
The new name is still a problem -- it sounds like a generic interface, but the diagnostic it emits is specific to attributes. That's going to be a maintenance issue.

I would make this an overload of `checkStringLiteralArgumentAttr()` instead of giving it a new name.


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

https://reviews.llvm.org/D114439