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