[PATCH] D116203: [clang] adds unary type transformations as compiler built-ins

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman accepted this revision.
aaron.ballman added a comment.

LGTM, though I did have a nit about diagnostic wording that you can feel free to take or leave at your discretion. Thank you for this work, this was a very large patch with a whole lot of review comments, but I think what we ended up with is really great!



================
Comment at: clang/include/clang/Sema/DeclSpec.h:424-431
+  static bool isTransformTypeTrait(TST T) {
+    constexpr TST Traits[] = {
+#define TRANSFORM_TYPE_TRAIT_DEF(_, Trait) TST_##Trait,
+#include "clang/AST/TransformTypeTraits.def"
+    };
+
+    return (T >= Traits[0] && T <= std::end(Traits)[-1]);
----------------
cjdb wrote:
> aaron.ballman wrote:
> > Errr, this isn't Python, so that `[-1]` terrifies me. It took me a good solid ten minutes to convince myself this was actually valid. Any interest in something along the lines of this form?
> I have many questions about the decisions I made in this snippet.
It was definitely a great way to wake up this morning. "Wait, what? WHAT?!?" followed by a bunch of furious standards reading. :-D


================
Comment at: clang/lib/AST/TypePrinter.cpp:1158
                                            raw_ostream &OS) {
   IncludeStrongLifetimeRAII Strong(Policy);
 }
----------------
cjdb wrote:
> aaron.ballman wrote:
> > cjdb wrote:
> > > rsmith wrote:
> > > > Remove this line too. No point building an RAII scope with nothing in it.
> > > Can we get rid of this function entirely in that case?
> > I believe you can.
> It looks like other things also have this as just empty, so maybe it's best to keep it?
That's fine by me!


================
Comment at: clang/test/SemaCXX/type-traits.cpp:3505
+  { using ExpectedError = __make_unsigned(unsigned _BitInt(1)); }
+  // expected-error@*:*{{'make_unsigned' is only compatible with non-bool integers and enum types, but was given 'unsigned _BitInt(1)'}}
+  { using ExpectedError = __make_unsigned(UnscopedBit); }
----------------
This diagnostic is a bit unfortunate because it was given a non-bool integer, so I don't know that users will know how to fix the issue from the text. I leave it to your discretion where to split that diagnostic up with a `%select` so that `_BitInt` is special or not; I don't think users will hit this diagnostic all that often, but I do have a little bit of a worry about template metaprogramming accidentally getting into this state.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116203