aaron.ballman accepted this revision.
aaron.ballman added a comment.
This revision is now accepted and ready to land.
LGTM, your choice on hiding in TokenKinds.def.
================
Comment at: clang/lib/Basic/ExpressionTraits.cpp:29-34
+ assert(T <= ET_Last && "invalid enum value!");
+ return ExpressionTraitNames[T];
+}
+
+const char *clang::getTraitSpelling(ExpressionTrait T) {
+ assert(T <= ET_Last && "invalid enum value!");
----------------
riccibruno wrote:
> aaron.ballman wrote:
> > Isn't `ET_Last` -1?
> Nope :) It's `-1` plus 1 per element in the enumeration. I have added the
> enumerators `ET_Last`, `ATT_Last` and `UETT_Last` for consistency with
> `UTT_Last`, `BTT_Last` and `TT_Last` which are needed.
>
> In this patch `ET_Last`, `ATT_Last` and `UETT_Last` are only used here in
> this assertion and could be replaced by the equivalent `T < XX_Last` where
> `XX_Last` is just added as the last element in the enumeration. However
> mixing `XX_Last = XX_LastElement` and `XX_Last = LastElement + 1` would be
> very error-prone.
Oh! I see now what's going on and that's clever; I was misunderstanding the
second macro usage (which makes me wonder if it would make more sense to hide
the `Last` fields at the bottom of TokenKinds.def rather than use the current
approach, but I don't insist). Thank you for the clarification.
================
Comment at: clang/lib/Sema/SemaExpr.cpp:3974
S.Diag(Loc, diag::ext_sizeof_alignof_function_type)
- << TraitKind << ArgRange;
+ << getTraitSpelling(TraitKind) << ArgRange;
return false;
----------------
riccibruno wrote:
> aaron.ballman wrote:
> > I think the original code was a bit more clear; would it make sense to make
> > the diagnostic engine aware of trait kinds so that it does this dance for
> > you? (It may be overkill given that we don't pass `UnaryExprOrTypeTrait`
> > objects to the diagnostic engine THAT often, but I'm curious what you
> > think.)
> I don't think it is worthwhile since as you say `UnaryExprOrTypeTrait`
> objects are not frequently passed to the diagnostic engine.
>
> Moreover I personally finds the explicit `getTraitSpelling(TraitKind)`
> clearer for two reasons:
> 1. Frequently a non-class enumerator is used as an index to a `select` in a
> diagnostic, relying on the implicit integer conversion. Special casing
> `UnaryExprOrTypeTrait` would be surprising.
>
> 2. (weaker) `<< TraitKind` could mean something else than the trait's
> spelling; for example this could print the trait's name or some user-visible
> version thereof.
Sold! Thank you. :-)
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D81455/new/
https://reviews.llvm.org/D81455
_______________________________________________
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
