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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
