[libcxx][type_traits] is_unsigned is false for enum types

[tmatheson]

Summary

Enum types are not arithmetic, therefore both is_arithmetic and is_unsigned
should be false for them. This change fixes the implementation of is_unsigned
(which was returning true for enums) and adds some more tests. Also adds a test
for enum class to is_arithmetic.

Note that std::is_unsigned is based on the implementation of is_unsigned,
which returns true for enum types. To avoid introducing version differences
between libcxx and clang, the implementation of is_unsigned is left unchanged
here, and an explicit check for __is_enum is added to the implementation of
is_unsigned instead.

clang-format to pass the linter.

[• Quuxplusone]

libcxx/include/type_traits

1461

FWIW, I would prefer to hew closer to the actual standardese in https://eel.is/c++draft/meta#tab:meta.unary.prop and just use __is_unsigned(_Tp) && is_arithmetic<_Tp>::value.

It's also definitely worth opening a discussion with compiler vendors — it'd be great if every compiler's __is_enum builtin were updated to reflect the standard semantics, since that is literally the entire point of that builtin. :P

libcxx/test/std/utilities/meta/meta.unary/meta.unary.prop/is_unsigned.pass.cpp

99

Surely the test for is_signed needs these same updates.

[• Quuxplusone]

libcxx/include/type_traits

1461

(oops, of course I meant s/__is_enum/__is_{un,}signed/ in my second paragraph)

[tmatheson]

Add corresponding tests to is_signed.pass.cpp

[tmatheson]

libcxx/include/type_traits

1461

I agree it would read better with is_arithmetic, but I got the impression from https://reviews.llvm.org/D67900 that we should be using builtins where to keep compile times down. is_arithmetic is already used for the alternative implementation below, in the #else // __has_keyword(__is_unsigned) block. So it would make both implementations very similar to switch.

[tmatheson]

libcxx/include/type_traits

1461

It's also definitely worth opening a discussion with compiler vendors — it'd be great if every compiler's __is_unsigned builtin were updated to reflect the standard semantics, since that is literally the entire point of that builtin. :P

Are there compilers other than clang that use the same spelling for the builtin? Introducing version incompatibilities seems worse to me than sticking with slightly weird builtin semantics, but it is worth discussing. The code as written should continue to work if the semantics of __is_unsigned are updated in the future.

[Mordante]

Why doesn't is_signed be updated in the same way? I see the builds on buildkite passing, but the difference between is_unsigned and is_signed look very odd to me.

libcxx/include/type_traits

1451

Please update the comments of the #else and #endif.

[tmatheson]

Add tests for enums with explicitly signed and unsigned underlying types

[tmatheson]

Update the comments on #else and #endif

[tmatheson]

In D97283#2582217, @Mordante wrote:

Why doesn't is_signed be updated in the same way? I see the builds on buildkite passing, but the difference between is_unsigned and is_signed look very odd to me.

is_signed is based on the __is_signed builtin, which unlike __is_unsigned is not broken for enums since clang 9 (according to the comment on the is_signed implementation).

You made me realise that I was missing a test that checks enums with signed and unsigned underlying types, so I have added those now.

[Mordante]

In D97283#2582244, @tmatheson wrote:

In D97283#2582217, @Mordante wrote:

Why doesn't is_signed be updated in the same way? I see the builds on buildkite passing, but the difference between is_unsigned and is_signed look very odd to me.

is_signed is based on the __is_signed builtin, which unlike __is_unsigned is not broken for enums since clang 9 (according to the comment on the is_signed implementation).

I read that for comment for is_signed and assumed it was a bug before clang 9 and only in __is_signed. But assumptions... seems not to be fixed for __is_unsigned yet.

[tmatheson]

@Quuxplusone @Mordante are there any outstanding issues to be addressed, or any further comments?

[• Quuxplusone]

I'm not an "accepter" for libc++ (wait for @ldionne?) but sure, LGTM if it LGThim.

libcxx/test/std/utilities/meta/meta.unary/meta.unary.comp/is_arithmetic.pass.cpp

102

I'd add your new EnumSigned and EnumUnsigned cases here, too.

[Mordante]

LGTM, but it still needs approval of one of the maintainers.

[tmatheson]

Thanks both, I'm still new to this process so didn't realise there were specific approvers.

[Mordante]

Yes libc++ has specific people to do the final approval. This isn't true for all parts of the LLVM project.

[Mordante]

We just changed the approvers for libc++. Now @Quuxplusone and my approval are a final approval. Feel free to commit this patch. If you don't have commit access please let me know your name and e-mail address and I'll commit on your behalf.

[ldionne]

Requesting changes because I'd like an answer to the question before we move forward with this.

libcxx/include/type_traits

1459–1460

Have we asked the compiler folks why __is_unsigned is implemented that way? This looks like a simple Clang bug to me.

@zoecarver did you implement this intrinsic or was it there already?

[Mordante]

It's a clang bug: clang/docs/LanguageExtensions.rst:1024

* ``__is_unsigned`` (C++, Embarcadero)
  Note that this currently returns true for enumeration types if the underlying
  type is unsigned, in violation of the requirements for ``std::is_unsigned``.
  This behavior is likely to change in a future version of Clang.

@zoecarver fixed the signed part in D67897

I want to look at the Clang site unless somebody beats me to it.

[ldionne]

In D97283#2605620, @Mordante wrote:

It's a clang bug: clang/docs/LanguageExtensions.rst:1024

* ``__is_unsigned`` (C++, Embarcadero)
  Note that this currently returns true for enumeration types if the underlying
  type is unsigned, in violation of the requirements for ``std::is_unsigned``.
  This behavior is likely to change in a future version of Clang.

@zoecarver fixed the signed part in D67897

I want to look at the Clang site unless somebody beats me to it.

I suggest we add the tests present in this patch, fix Clang, and then make libc++ conditionally use just __is_unsigned or a combination of __is_unsigned && !__is_enum based on the version of Clang. We'll be able to remove that workaround in ~6 months due to the compiler policy, which makes it more acceptable. So:

template <class _Tp>
struct _LIBCPP_TEMPLATE_VIS is_unsigned
    : _BoolConstant<
#if _LIBCPP_CLANG_VER < whateverversion // Clang prior to <whateverversion> has a bug and treats enumerations as unsigned
    __is_unsigned(_Tp) && !__is_enum(_Tp)
#else
    __is_unsigned(_Tp)
#endif
> {};

#if _LIBCPP_STD_VER > 14 && !defined(_LIBCPP_HAS_NO_VARIABLE_TEMPLATES)
template <class _Tp>
_LIBCPP_INLINE_VAR _LIBCPP_CONSTEXPR bool is_unsigned_v = is_unsigned<_Tp>::value;
#endif

All those tests can be done in this very patch, BTW.

[tmatheson]

make libc++ conditionally use just __is_unsigned or a combination of __is_unsigned && !__is_enum based on the version of Clang

That's exactly what I was trying to avoid :) I'm reluctant to change the builtin for the following reasons:

• Since this is marked as an Embarcadero builtin, it might break whatever that is and I'm not sure who to ask about it
• I didn't know what the compiler support policy is so was hedging on the side of not breaking the interface
• __is_unsigned && !__is_enum will work for both builtin behaviours
• Keep the patch small

If this is the preferred approach though and the above are not concerns, I can also change the builtin and add the extra #ifdef.

[ldionne]

In D97283#2606794, @tmatheson wrote:

make libc++ conditionally use just __is_unsigned or a combination of __is_unsigned && !__is_enum based on the version of Clang

That's exactly what I was trying to avoid :) I'm reluctant to change the builtin for the following reasons:

• Since this is marked as an Embarcadero builtin, it might break whatever that is and I'm not sure who to ask about it
• I didn't know what the compiler support policy is so was hedging on the side of not breaking the interface
• __is_unsigned && !__is_enum will work for both builtin behaviours
• Keep the patch small

If this is the preferred approach though and the above are not concerns, I can also change the builtin and add the extra #ifdef.

That's my preferred approach, however I think we need to bring in someone from Clang like @rsmith to discuss this. Richard, currently, the __is_unsigned builtin returns true for enumeration types - do you think it's reasonable to fix that bug, or should we keep it to avoid breaking folks who might rely on it?

IMO: If we think it's a bug in the standard library that std::is_unsigned returns true for enumerations, and if we think it's serious enough that we want to fix it (even though it might break some people who were relying on the old behaviour in tortuous ways), then I think we should fix it in Clang too. Having the two disagree does not improve anything (in fact libc++ might be amongst the only users of __is_unsigned). That's my opinion, but I'd like to know yours.

[Mordante]

In D97283#2606794, @tmatheson wrote:

make libc++ conditionally use just __is_unsigned or a combination of __is_unsigned && !__is_enum based on the version of Clang

• Since this is marked as an Embarcadero builtin, it might break whatever that is and I'm not sure who to ask about it

__is_signed is also as an Embarcadero builtin and we modified that builtin.

• Keep the patch small

I'm in favour of that too.

If this is the preferred approach though and the above are not concerns, I can also change the builtin and add the extra #ifdef.

I also prefer to fix Clang. I thought we could first land this patch and then fix Clang. But I don't object against fixing Clang first.
I think we can make two separate patches and first process the Clang fix and then the libc++ part.

[zoecarver]

libcxx/include/type_traits

1459–1460

Yeah, I fixed this for __is_signed. I think I just need to update __is_unsigned as well. I might be able to put up a patch for that later today.

[zoecarver]

Thank you for adding all these tests, they're great!

@tmatheson was this causing problems for you in the wild, or did you just notice that there was an issue? If the latter, I think we should just take the tests from this patch and only enable them on ToT clang for the next six months. That way we don't add (more) branches to this code that we'll inevitably forget to remove in six months.

libcxx/include/type_traits

1459–1460

Here you go: D98104.

[tmatheson]

• Add #ifdef around the __is_unsigned condition based on clang version number. I'm not convinced the value of this outweighs the noise it creates. But with it, this patch can be landed before or after D98104.

• Add EnumSigned and EnumUnsigned to is_arithmetic tests.

[tmatheson]

@zoecarver

was this causing problems for you in the wild, or did you just notice that there was an issue? If the latter, I think we should just take the tests from this patch and only enable them on ToT clang for the next six months.

I believe this originally came up because it failed in a C++ validation suite in our CI, which falls into the "just noticed it" category. I've added the branches but equally happy to remove them again if needed.

[tmatheson]

Ping

[• Quuxplusone]

Ping @zoecarver because I think he needs to be one of the approvers here. But once it LGTH, I'm happy to be the second accepter.

libcxx/include/type_traits

1452–1453

AFAIK, _LIBCPP_CLANG_VER >= 1300 logically implies __has_keyword(__is_enum). Therefore, just testing the latter is sufficient. So I was going to suggest the edit to

#if __has_keyword(__is_unsigned) && __has_keyword(__is_enum)

However,

• On compilers which are Clang, all supported Clangs definitely have __is_enum.
• On compilers which are not Clang, we never need to use __is_enum.

So it doesn't seem that that part of the condition is actually used, either.
I think the original condition #if __has_keyword(__is_unsigned) is now the correct condition. (And revert the comments on the #else and #endif lines accordingly.)

1459–1461

Please duplicate the #if _LIBCPP_CLANG_VER < 1300 workaround here, i.e., don't bother to use __is_enum unless we're on Clang <13.

[• Quuxplusone]

libcxx/test/std/utilities/meta/meta.unary/meta.unary.prop/is_unsigned.pass.cpp

103

Oh, you added these new tests, that's why AArch is failing!
Plain char (and apparently plain wchar_t? TIL) have implementation-defined signedness.
I think it would be good to at least verify that

static_assert(is_signed_v<char> != is_unsigned_v<char>);
static_assert(is_signed_v<wchar_t> != is_unsigned_v<wchar_t>);

but it would also be OK with me to just remove these two new lines from the tests. What do other people think?

[tmatheson]

Remove char and wchar_t tests

[zoecarver]

Thank you for adding all these tests!

libcxx/include/type_traits

1451

With other builtins that have had issues in earlier versions of Clang, such as __is_pointer, we simply disable the fast path before Clang XX. I think we should do that here too. For the most part, the people who care about this type of performance are using ToT. Also, this will greatly reduce the complexity here, so we don't add any more branches to the already complicated implementation. WDYT?

libcxx/test/std/utilities/meta/meta.unary/meta.unary.prop/is_signed.pass.cpp

58

I know this wasn't you, but I really don't like the name NotEmpty. Maybe call this VirtualDestructor or something.

[• Quuxplusone]

libcxx/include/type_traits

1451

FWIW, I don't object to that idea at all.

libcxx/test/std/utilities/meta/meta.unary/meta.unary.prop/is_signed.pass.cpp

58

This type is cut-and-pasted all over the place, so please change it everywhere (which means change it in a separate PR) or not at all.

[tmatheson]

Disable fast path for clang < 13

[tmatheson]

libcxx/test/std/utilities/meta/meta.unary/meta.unary.prop/is_signed.pass.cpp

58

I'll leave it as NotEmpty since it is called that elsewhere

[• Quuxplusone]

Unfortunately I think the current check for

#if __has_keyword(__is_unsigned) && _LIBCPP_CLANG_VER >= 1300

has the effect of disabling the fast path for GCC and MSVC forever.
However, we currently have the exact same problem in checks for __is_pointer and __is_fundamental (plus a redundant check for nullptr support in the latter), so I support landing this patch as-is-now and then I'll make a followup PR myself to change all these checks at once to e.g.

#if __has_keyword(__is_unsigned) && (!defined(_LIBCPP_CLANG_VER) || (_LIBCPP_CLANG_VER >= 1300))

[zoecarver]

@tmatheson This looks great. Thank you for enduring all the back and forth. Assuming the tests pass, let's land it!

In D97283#2629143, @Quuxplusone wrote:

Unfortunately I think the current check for

#if __has_keyword(__is_unsigned) && _LIBCPP_CLANG_VER >= 1300

has the effect of disabling the fast path for GCC and MSVC forever.
However, we currently have the exact same problem in checks for __is_pointer and __is_fundamental (plus a redundant check for nullptr support in the latter), so I support landing this patch as-is-now and then I'll make a followup PR myself to change all these checks at once to e.g.

#if __has_keyword(__is_unsigned) && (!defined(_LIBCPP_CLANG_VER) || (_LIBCPP_CLANG_VER >= 1300))

I'd also be OK just waiting 6 months and removing these paths altogether.

[tmatheson]

Thanks. Should I wait for @ldionne to approve?

[zoecarver]

Thanks. Should I wait for @ldionne to approve?

Thanks for asking. Louis is out for the months, so as long as two of myself, Arthur, Marek, or Mark approve, you're good to go.

[zoecarver]

@tmatheson Thanks again for working on this patch. In the future, please make sure you let the pre-commit builds finish. That way, if there is an issue, we'll catch it before the patch lands. No worries this time, we all forget (I have certainly forgotten more than most). Just wanted to let you know going forward.

[tmatheson]

@zoecarver yes sorry about that, for some reason I had it in my head that they were green. I'll keep an eye out in case I broke something.

libcxx/include/type_traits

 _LIBCPP_INLINE_VAR _LIBCPP_CONSTEXPR bool is_signed_v
     = is_signed<_Tp>::value;
 #endif
 
 #endif // __has_keyword(__is_signed)
 
 // is_unsigned
 
-#if __has_keyword(__is_unsigned)
+#if __has_keyword(__is_unsigned) && __has_keyword(__is_enum)
 
-template<class _Tp>
-struct _LIBCPP_TEMPLATE_VIS is_unsigned : _BoolConstant<__is_unsigned(_Tp)> { };
+template <class _Tp>
+struct _LIBCPP_TEMPLATE_VIS is_unsigned
+    : _BoolConstant<__is_unsigned(_Tp) && !__is_enum(_Tp)> {};
 
 #if _LIBCPP_STD_VER > 14 && !defined(_LIBCPP_HAS_NO_VARIABLE_TEMPLATES)
 template <class _Tp>
-_LIBCPP_INLINE_VAR _LIBCPP_CONSTEXPR bool is_unsigned_v = __is_unsigned(_Tp);
+_LIBCPP_INLINE_VAR _LIBCPP_CONSTEXPR bool is_unsigned_v = __is_unsigned(_Tp) &&
+                                                          !__is_enum(_Tp);
 #endif
 
 #else // __has_keyword(__is_unsigned)
 
 template <class _Tp, bool = is_integral<_Tp>::value>
 struct __libcpp_is_unsigned_impl : public _LIBCPP_BOOL_CONSTANT(_Tp(0) < _Tp(-1)) {};
 
 template <class _Tp>
 struct __libcpp_is_unsigned_impl<_Tp, false> : public false_type {};  // floating point

libcxx/test/std/utilities/meta/meta.unary/meta.unary.comp/is_arithmetic.pass.cpp

 
 class Abstract
 {
     virtual ~Abstract() = 0;
 };
 
 enum Enum {zero, one};
 
+enum class EnumClass { zero, one };
+
 typedef void (*FunctionPtr)();
 
 
 int main(int, char**)
 {
     test_is_arithmetic<short>();
     test_is_arithmetic<unsigned short>();
     test_is_arithmetic<int>();
int main(int, char**)
     test_is_not_arithmetic<int&>();
     test_is_not_arithmetic<int&&>();
     test_is_not_arithmetic<int*>();
     test_is_not_arithmetic<const int*>();
     test_is_not_arithmetic<char[3]>();
     test_is_not_arithmetic<char[]>();
     test_is_not_arithmetic<Union>();
     test_is_not_arithmetic<Enum>();
+    test_is_not_arithmetic<EnumClass>();
     test_is_not_arithmetic<FunctionPtr>();
     test_is_not_arithmetic<Empty>();
     test_is_not_arithmetic<incomplete_type>();
     test_is_not_arithmetic<bit_zero>();
     test_is_not_arithmetic<NotEmpty>();
     test_is_not_arithmetic<Abstract>();
 
   return 0;
 }

libcxx/test/std/utilities/meta/meta.unary/meta.unary.prop/is_signed.pass.cpp

 class Class
 {
 public:
     ~Class();
 };
 
 struct A; // incomplete
 
+class incomplete_type;
+
+class Empty {};
+
+class NotEmpty {
+  virtual ~NotEmpty();
+};
+
+union Union {};
+
+struct bit_zero {
+  int : 0;
+};
+
+class Abstract {
+  virtual ~Abstract() = 0;
+};
+
+enum Enum { zero, one };
+
+enum class EnumClass { zero, one };
+
+typedef void (*FunctionPtr)();
+
 int main(int, char**)
 {
-    test_is_not_signed<void>();
-    test_is_not_signed<int&>();
-    test_is_not_signed<Class>();
-    test_is_not_signed<int*>();
-    test_is_not_signed<const int*>();
-    test_is_not_signed<char[3]>();
-    test_is_not_signed<char[]>();
-    test_is_not_signed<bool>();
-    test_is_not_signed<unsigned>();
-    test_is_not_signed<A>();
-
-    test_is_signed<int>();
-    test_is_signed<double>();
+  // Cases where !is_arithmetic implies !is_signed
+  test_is_not_signed<std::nullptr_t>();
+  test_is_not_signed<int&&>();
+  test_is_not_signed<Union>();
+  test_is_not_signed<Enum>();
+  test_is_not_signed<EnumClass>();
+  test_is_not_signed<FunctionPtr>();
+  test_is_not_signed<Empty>();
+  test_is_not_signed<incomplete_type>();
+  test_is_not_signed<bit_zero>();
+  test_is_not_signed<NotEmpty>();
+  test_is_not_signed<Abstract>();
+
+  test_is_signed<char>();
+  test_is_signed<signed char>();
+  test_is_signed<wchar_t>();
+  test_is_signed<short>();
+  test_is_signed<long>();
+  test_is_signed<float>();
 
+  test_is_not_signed<unsigned char>();
+  test_is_not_signed<unsigned short>();
+  test_is_not_signed<unsigned int>();
+  test_is_not_signed<unsigned long>();
+
+  test_is_not_signed<bool>();
+  test_is_not_signed<unsigned>();
+
 #ifndef _LIBCPP_HAS_NO_INT128
     test_is_signed<__int128_t>();
     test_is_not_signed<__uint128_t>();
 #endif
 
   return 0;
 }

libcxx/test/std/utilities/meta/meta.unary/meta.unary.prop/is_unsigned.pass.cpp

 class Class
 {
 public:
     ~Class();
 };
 
 struct A; // incomplete
 
+class incomplete_type;
+
+class Empty {};
+
+class NotEmpty {
+  virtual ~NotEmpty();
+};
+
+union Union {};
+
+struct bit_zero {
+  int : 0;
+};
+
+class Abstract {
+  virtual ~Abstract() = 0;
+};
+
+enum Enum { zero, one };
+
+enum class EnumClass { zero, one };
+
+typedef void (*FunctionPtr)();
+
 int main(int, char**)
 {
-    test_is_not_unsigned<void>();
-    test_is_not_unsigned<int&>();
-    test_is_not_unsigned<Class>();
-    test_is_not_unsigned<int*>();
-    test_is_not_unsigned<const int*>();
-    test_is_not_unsigned<char[3]>();
-    test_is_not_unsigned<char[]>();
-    test_is_not_unsigned<int>();
-    test_is_not_unsigned<double>();
-    test_is_not_unsigned<A>();
+  // Cases where !is_arithmetic implies !is_unsigned
+  test_is_not_unsigned<std::nullptr_t>();
+  test_is_not_unsigned<int&&>();
+  test_is_not_unsigned<Union>();
+  test_is_not_unsigned<Enum>();
+  test_is_not_unsigned<EnumClass>();
+  test_is_not_unsigned<FunctionPtr>();
+  test_is_not_unsigned<Empty>();
+  test_is_not_unsigned<incomplete_type>();
+  test_is_not_unsigned<A>();
+  test_is_not_unsigned<bit_zero>();
+  test_is_not_unsigned<NotEmpty>();
+  test_is_not_unsigned<Abstract>();
+
+  test_is_not_unsigned<char>();
+  test_is_not_unsigned<signed char>();
+  test_is_not_unsigned<wchar_t>();
+  test_is_not_unsigned<short>();
+  test_is_not_unsigned<long>();
+  test_is_not_unsigned<float>();
+
+  test_is_unsigned<unsigned char>();
+  test_is_unsigned<unsigned short>();
+  test_is_unsigned<unsigned int>();
+  test_is_unsigned<unsigned long>();
 
-    test_is_unsigned<bool>();
-    test_is_unsigned<unsigned>();
 
 #ifndef _LIBCPP_HAS_NO_INT128
     test_is_unsigned<__uint128_t>();
     test_is_not_unsigned<__int128_t>();
 #endif
 
   return 0;
 }