[libcxx-commits] [PATCH] D97283: [libcxx][type_traits] is_unsigned is false for enum types

[Arthur O'Dwyer via Phabricator via libcxx-commits]

Quuxplusone requested changes to this revision.
Quuxplusone added a comment.
This revision now requires changes to proceed.

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.



================
Comment at: libcxx/include/type_traits:1455-1456
+// Before clang 13 __is_unsigned returned true for enums with signed underlying type
+#if __has_keyword(__is_unsigned) &&                                            \
+    (_LIBCPP_CLANG_VER >= 1300 || __has_keyword(__is_enum))
 
----------------
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.)


================
Comment at: libcxx/include/type_traits:1471-1472
 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
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D97283