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