[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/bit:84
 {
-    static_assert(__bitop_unsigned_integer<_Tp>::value, "__rotl requires unsigned");
+    static_assert(is_unsigned<_Tp>::value, "__rotl requires an unsigned integer type");
     const unsigned int __dig = numeric_limits<_Tp>::digits;
----------------
I relaxed the static_assert on the internal functions for the same (bad) intuitive reason that the internal functions are provided in the first place. We never actually //use// any of these internal functions, but //if we did//, it might be nice to be able to use them on, I dunno, `char` or something. I'm highly ambivalent about whether we should be asserting anything at all, or if we do, what it should be. So I'll take suggestions if anyone's got any.


================
Comment at: libcxx/include/bit:119-125
         const unsigned int __ulldigits = numeric_limits<unsigned long long>::digits;
-        while ((__iter = __libcpp_ctz(static_cast<unsigned long long>(__t))) == __ulldigits)
+        while (static_cast<unsigned long long>(__t) == 0uLL)
         {
-            __ret += __iter;
+            __ret += __ulldigits;
             __t >>= __ulldigits;
         }
+        return __ret + __libcpp_ctz(static_cast<unsigned long long>(__t));
----------------
This is a bugfix: `std::countr_zero(__uint128_t(1) << 64)` had been trying to evaluate `__libcpp_ctz(0uLL)` in constexpr context, and that's not allowed. The old tests didn't catch it but the new tests do.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D102328