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 is correct. Name is "Steffen Larsen" and email is
"steffen.lar...@intel.com". Thanks a ton. 😄
================
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:
> 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.)
`isParamExpr` is fine by me! I have also added the suggested comment. Thanks!
(Also, good eye on the `//` vs `///`)
================
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:
> 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.
Overloading `checkStringLiteralArgumentAttr` is a good shout. I have done that.
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
