[PATCH] D81455: [clang][NFC] Generate the {Type,ArrayType,UnaryExprOrType,Expression}Traits enumerations from TokenKinds.def...

[Bruno Ricci via Phabricator via cfe-commits]

riccibruno marked 2 inline comments as done.
riccibruno added a comment.

Thanks for the review Aaron! I have added some comments inline.



================
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!");
----------------
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.


================
Comment at: clang/lib/Sema/SemaExpr.cpp:3974
     S.Diag(Loc, diag::ext_sizeof_alignof_function_type)
-      << TraitKind << ArgRange;
+        << getTraitSpelling(TraitKind) << ArgRange;
     return false;
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D81455