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

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith accepted this revision.
rsmith added a comment.

A handful more comments, but I think we've basically converged here. I'm happy to take another look after you address these if you'd like (or you could ask someone else for a final pass), or for you to land this once you're happy.

Before landing, it'd be good to patch libc++ to use these intrinsics and run its testsuite to look for any unexpected behavior changes, if you've not already done so with this version of the patch.



================
Comment at: clang/lib/AST/ASTContext.cpp:10811
   case BuiltinType::Long:
+  case BuiltinType::ULong:
     return UnsignedLongTy;
----------------
I think we don't need this case any more.


================
Comment at: clang/lib/Parse/ParseDeclCXX.cpp:1149-1150
+  default:
+    assert(false && "passed in an unhandled type transformation built-in");
+    llvm_unreachable("passed in an unhandled type transformation built-in");
+  }
----------------
rsmith wrote:
> We don't need both of these. Just the `llvm_unreachable` would suffice.
This comment still needs to be addressed.


================
Comment at: clang/lib/Parse/ParseExpr.cpp:1066-1067
 
+          REVERTIBLE_TYPE_TRAIT(__add_lvalue_reference);
+          REVERTIBLE_TYPE_TRAIT(__add_rvalue_reference);
           REVERTIBLE_TYPE_TRAIT(__is_abstract);
----------------
Just curious: why do we only handle six of the traits here, rather than all of them?


================
Comment at: clang/lib/Sema/SemaType.cpp:9359
+                                         SourceLocation Loc) {
+  static const std::array<CanQualType *, 6> SignedIntegers = {
+      &S.Context.SignedCharTy, &S.Context.ShortTy,    &S.Context.IntTy,
----------------
Remove the `static` -- it's not correct to use a `static` local for this. There can be multiple `Sema` instances, and this will pick the types from whichever one was used the first time this function was called.


================
Comment at: clang/lib/Sema/SemaType.cpp:9361-9365
+      &S.Context.LongTy,       &S.Context.LongLongTy, &S.Context.Int128Ty};
+  static const std::array<CanQualType *, 6> UnsignedIntegers = {
+      &S.Context.UnsignedCharTy,     &S.Context.UnsignedShortTy,
+      &S.Context.UnsignedIntTy,      &S.Context.UnsignedLongTy,
+      &S.Context.UnsignedLongLongTy, &S.Context.UnsignedInt128Ty};
----------------
We don't have an `__int128` on 32-bit targets. Maybe below you can form an `ArrayRef` and slice off the last element if `__int128` is unsupported?


================
Comment at: clang/lib/Sema/SemaType.cpp:9372
+      llvm::find_if(*Consider, [&S, &BaseType](const CanQual<Type> *T) {
+        return S.Context.getTypeSize(BaseType) ==
+               S.Context.getTypeSize(T->getTypePtr());
----------------
Please only compute the size of `BaseType` once.


================
Comment at: clang/lib/Sema/SemaType.cpp:9375
+      });
+  assert(Result != Consider->end());
+
----------------
Can this fail for an enum whose underlying type is `_BitInt(N)` for some unusual `N`?


================
Comment at: clang/lib/Sema/SemaType.cpp:9392-9393
+       BaseType->isWideCharType() ||
+       (BaseType->isEnumeralType() &&
+        !GetEnumUnderlyingType(*this, BaseType, {})->isBitIntType()));
 
----------------
I've been pondering the proper behavior for `make_signed` on `enum E : _BitInt(N) {}`. I think the standard wording here is broken (see my email to the lib reflector, subject "make_signed / make_unsigned and padding / extended integer types"), but I'm not certain what the right rule is. Let's say that always producing a `_BitInt` type in that case is good enough for now. :)


================
Comment at: clang/lib/Sema/SemaType.cpp:9407
+    return Context.getUnaryTransformType(BaseType, BaseType, UKind);
+  switch (UKind) {
+  case UnaryTransformType::EnumUnderlyingType:
----------------
Have you considered moving the calls to `getUnaryTransformType` into this function? Right now they're scattered among the `Builtin*` helper functions, and it's hard to be sure that every code path calls it. I think it'd be a lot simpler to create the `UnaryTransformType` wrapper once, in a single place.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116203