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