[libcxx-commits] [PATCH] D102328: [libc++] Fix __bitop_unsigned_integer, rename to __is_unsigned_integer, and fix constexprness of std::countr_zero(__uint128_t)

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

Quuxplusone added inline comments.


================
Comment at: libcxx/include/type_traits:816
+#ifndef _LIBCPP_HAS_NO_INT128
+template <> struct __is_unsigned_integer<__uint128_t>        : public true_type {};
+#endif
----------------
Mordante wrote:
> How do you feel about using `__libcpp_is_signed_integer` to match the `__libcpp_is_floating_point` below?
I wasn't sure what the convention is here. It looks like we can't use e.g. `__is_floating_point` because that's sometimes a builtin, so we use `__libcpp_is_floating_point` instead? In this case `__is_signed_integer` isn't a builtin — well, isn't one //today// — but yeah, I guess on this second glance I am coming around to naming this entrypoint with a leading `__libcpp_`. That way, if it ever becomes a builtin, we won't have to change its call-sites.


================
Comment at: libcxx/test/std/numerics/bit/bit.pow.two/bit_ceil.pass.cpp:60
+#ifndef _LIBCPP_HAS_NO_INT128
+constexpr bool test_uint128()
 {
----------------
Mordante wrote:
> What about removing this function and add it to the generic test and use it in a `if constexpr(std::same_as<T, __uint128_t>)` block?
> (Obviously still guarded by`#ifndef _LIBCPP_HAS_NO_INT128`)
Part of my thinking here was that these tests could be ported over into `libcxx/test/libcxx/` to test `std::__bit_ceil` and so on, in pre-'17 language modes, so we want to keep avoiding `if constexpr`.
But clearly I didn't actually //do// that porting-over; and I did remove the trailing `,""` from all the static_asserts; so I guess it'd be okay to move this into `test()`. It would make the file a bit shorter. Will do, or at least consider.



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D102328