[PATCH] D116203: [clang] adds unary type transformations as compiler built-ins
Christopher Di Bella via Phabricator via cfe-commits
cfe-commits at lists.llvm.org
Fri May 13 12:56:48 PDT 2022
cjdb added inline comments.
================
Comment at: clang/lib/Parse/ParseDecl.cpp:4155
+#undef TRANSFORM_TYPE_TRAIT_DEF
+ if (!ParseTypeTransformTypeSpecifier(DS))
+ goto ParseIdentifier;
----------------
rsmith wrote:
> The name signature you're using for this function is inconsistent with the conventions in the rest of the parser: when a `Parser::Parse...` function with a `bool` return type returns `true`, it means "I have failed and issued a diagnostic". For "parse this if possible and return whether you did", we usually use `Parser::MaybeParse...` (eg, `MaybeParseAttributes`).
>
> Alternatively you could do the one-token lookahead here. If the `setKind` call is here, it'll be a lot clearer why it makes sense to `goto ParseIdentifier;`.
>
> Also, for workarounds for a standard library issue, we normally include a `// HACK:` comment explaining what we're doing and why.
Comment added, but I'd prefer to keep the function call so it keeps the logic self-contained and named.
================
Comment at: clang/lib/Sema/SemaType.cpp:9154
+ constexpr auto UKind = UTTKind::RemovePointer;
+ if (!BaseType->isPointerType() && !BaseType->isObjCObjectPointerType())
+ return Context.getUnaryTransformType(BaseType, BaseType, UKind);
----------------
rsmith wrote:
> This is `!BaseType->isAnyPointerType()`.
>
> What about block pointers? I think this patch is doing the right thing here, by saying that this only applies to pointers spelled with `*` (plain and obj-c pointers) and not to pointers spelled with `^`, but it seems worth calling out to ensure that I'm not the only one who's thought about it :)
Done, and added a test.
================
Comment at: clang/lib/Sema/SemaType.cpp:9251
+ BaseType->isBooleanType()) {
+ Diag(Loc, diag::err_make_signed_integral_only) << IsMakeSigned << BaseType;
+ return QualType();
----------------
rsmith wrote:
> Should we support vector types here?
Is it a conforming extension for `make_signed_t<int __attribute__((ext_vector_type(2)))>` to work?
================
Comment at: clang/lib/Sema/SemaType.cpp:9265-9266
+ ? Context.getBitIntType(!IsMakeSigned, Context.getIntWidth(BaseType))
+ : Context.getIntTypeForBitwidth(Context.getIntWidth(BaseType),
+ IsMakeSigned);
+ Qualifiers Quals = Underlying.getQualifiers();
----------------
rsmith wrote:
> This is wrong: if, say, `int` and `long` are the same bit-width, this will give the same type for `__make_unsigned(int)` and `__make_unsigned(long)`, where they are required to produce `unsigned int` and `unsigned long` respectively.
>
> Please look at `Context.getCorrespondingSignedType` and `Context.getCorrespondingUnsignedType` to see how to do this properly; you may be able to call those functions directly in some cases, but be careful about the cases where we're required to return the lowest-rank int type of the given size. Note that `getIntTypeForBitwidth` does *not* do that; rather, it produces the *preferred* type of the given width, and for WebAssembly and AVR it produces something other than the lowest-rank type in practice in some cases.
This makes me very happy.
================
Comment at: clang/lib/Sema/SemaType.cpp:9267-9270
+ Qualifiers Quals = Underlying.getQualifiers();
+ Quals.setCVRQualifiers(BaseType.getCVRQualifiers());
+ Underlying = QualType(Underlying.getSplitUnqualifiedType().Ty,
+ Quals.getAsOpaqueValue());
----------------
rsmith wrote:
> As before, the opaque value is, well, opaque, and you shouldn't be using it in this way. Also, you just created `Underlying` so you know it's unqualified, so there's no need to ask for its qualifiers.
>
> See suggested edit. Alternatively (#1), if you want to preserve all qualifiers:
>
> ```
> Underlying = Context.getQualifiedType(BaseType.getQualifiers());
> ```
>
> Alternatively (#2) we could strictly follow the standard wording and preserve only cv-qualifiers and not `restrict`. I think preserving all qualifiers is more in line with the intent, but preserving only `const` and `volatile` is probably the best match for what a C++ implementation of the type trait would do. *shrug*
As with `decay`, I think it's best if we pretend `restrict` is a standard qualifier, either for everything or for nothing. However, I don't think that `restrict` is a valid contender here. Isn't it only applicable to pointers, which aren't transformed by this pair?
================
Comment at: clang/lib/Sema/SemaType.cpp:9136
+ Qualifiers Quals = Underlying.getQualifiers();
+ Quals.removeCVRQualifiers();
+ return Context.getUnaryTransformType(
----------------
rsmith wrote:
> cjdb wrote:
> > aaron.ballman wrote:
> > > What about things like type attributes (are those lost during decay)?
> > According to https://eel.is/c++draft/tab:meta.trans.other, no.
> Type attributes are non-standard, so we can't answer this question by looking at the standard. Nonetheless, doing what `getDecayedType` does seems like the best choice.
I think this is a no-op?
================
Comment at: clang/test/CodeGenCXX/mangle.cpp:54
// CHECK-LABEL: define{{.*}} void @_Z1fno
-void f(__int128_t, __uint128_t) { }
+void f(__int128_t, __uint128_t) {}
----------------
rsmith wrote:
> If it's easy, can you undo the unrelated formatting changes in this file? That'll make future archaeology a little easier and help reduce the chance of merge conflicts. (Not a bit deal though.)
git-clang-format seems to insist these are formatted despite the lines not being touched. I'm not fond of arguing with clang-format.
================
Comment at: clang/test/CodeGenCXX/mangle.cpp:1113
template void fn<E>(E, __underlying_type(E));
-// CHECK-LABEL: @_ZN6test552fnINS_1EEEEvT_U3eutS2_
+// CHECK-LABEL: @_ZN6test552fnINS_1EEEEvT_Uu17__underlying_typeS2_
}
----------------
rsmith wrote:
> Do we have a corresponding test for the MS ABI?
Per offline discussion, I think this is no longer being requested?
================
Comment at: clang/test/SemaCXX/remove_pointer.mm:7
+
+template <class T> using remove_pointer_t = __remove_pointer(T);
+
----------------
rsmith wrote:
> Is there a reason for this wrapper? You're directly using `__is_same` below, why not directly use `__remove_pointer` too?
The rationale here is that we can assign `__remove_pointer`, etc., to a using declaration, which is how they're exposed to users.
================
Comment at: clang/test/SemaCXX/remove_pointer.mm:12-46
+void remove_pointer() {
+ { int a[T(__is_same(remove_pointer_t<void>, void))]; }
+ { int a[T(__is_same(remove_pointer_t<const void>, const void))]; }
+ { int a[T(__is_same(remove_pointer_t<volatile void>, volatile void))]; }
+ { int a[T(__is_same(remove_pointer_t<const volatile void>, const volatile void))]; }
+ { int a[T(__is_same(remove_pointer_t<int>, int))]; }
+ { int a[T(__is_same(remove_pointer_t<const int>, const int))]; }
----------------
rsmith wrote:
> I expected to see some tests for ObjC pointers in here, but I don't see any. I'd like to see `__remove_pointer(id)` and `@class X; static_assert(__is_same(__remove_pointer(X*, X)));`
Not knowing a lick of ObjC this was a fun one to get working (I didn't know that `@class X` had to be global).
================
Comment at: clang/test/SemaCXX/type-traits.cpp:2862
+struct S {};
+template <class T> using remove_const_t = __remove_const(T);
+
----------------
rsmith wrote:
> Consider using the trait directly instead of wrapping it in an alias.
Same as in `remove_pointer.mm`.
================
Comment at: clang/test/SemaCXX/type-traits.cpp:3099-3100
+
+ { int a[T(__is_same(remove_pointer_t<int __attribute__((address_space(1))) *>, int))]; }
+ { int a[T(__is_same(remove_pointer_t<S __attribute__((address_space(2))) *>, S))]; }
+}
----------------
rsmith wrote:
> It seems strange to remove the address space qualifier here, given that this trait doesn't remove cv-qualifiers.
Does `int __attribute__((address_space(1)))` make sense as a type? I thought it had to be a pointee.
================
Comment at: clang/test/SemaCXX/type-traits.cpp:3269
+ { int a[T(__is_same(remove_cvref_t<int S::*const volatile>, int S::*))]; }
+ { int a[T(__is_same(remove_cvref_t<int (S::*const volatile)()>, int(S::*)()))]; }
+ { int a[T(__is_same(remove_cvref_t<int (S::*const volatile)() &>, int(S::*)() &))]; }
----------------
rsmith wrote:
> The tests you have here for references to pointers-to-members seem excessive (throughout); I don't think there's any reason to think that a reference to pointer-to-member would be so different from a reference to any other type that would justify this level of testing of the former case.
>
> The cases that seem interesting here are the ones for non-referenceable types, but those are covered separately below.
I've removed all but one pointer-to-member function.
================
Comment at: clang/test/SemaCXX/type-traits.cpp:3391
+ static void checks() {
+ { int a[T(__is_same(add_lvalue_reference_t<M>, M))]; }
+ { int a[T(__is_same(add_pointer_t<M>, M))]; }
----------------
rsmith wrote:
> It's weird that we permit an abominable function type to show up in the argument of a trait during template instantiation but not when written directly. That behavior should really be consistent, and I think we should consistently allow it. That is, we should allow:
>
> `static_assert(__is_same(__add_pointer(int () &), int () &));`
>
> ... just like we allow it if you hide the usage of the traits inside a template. That'd make this easier to test, too: you can then put these tests with the other tests for the corresponding traits. (This should only require passing `DeclaratorContext::TemplateArg` to `ParseTypeName` instead of the default of `DeclaratorContext::TypeName` in the places where we parse type traits, reflecting that we want to use the rules for template arguments in these contexts.) This would also fix an incompatibility between GCC and Clang: https://godbolt.org/z/GdxThdvjz
>
> That said, I don't think we need to block this patch on this change, so if you'd prefer to not do this now, that's fine :)
Gonna defer to a separate CL, but will get it done one day soon :-)
================
Comment at: clang/test/SemaCXX/type-traits.cpp:3495-3496
+
+ { int a[T(__is_same(make_signed_t<Enum>, int))]; }
+ { int a[T(__is_same(make_signed_t<SignedEnum>, int))]; }
+ { int a[T(__is_same(make_signed_t<const SignedEnum>, const int))]; }
----------------
rsmith wrote:
> It'd be useful to test enums with different underlying types. However, this test is not portable: if `short` and `int` are the same size, this is required to produce `short`, not `int`. It'd be good to have some test coverage of that quirk too. Perhaps this is easiest to see with a test like:
>
> ```
> enum E : long long {};
> static_assert(__is_same(__make_signed(E), long));
> ```
>
> ... which should hold in cases where `long` and `long long` are the same size and are larger than `int`.
I agree, but what about when `long` is smaller than `int`?
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D116203/new/
https://reviews.llvm.org/D116203
More information about the cfe-commits
mailing list