[PATCH] D51262: Implement P0553 and P0556

[Marshall Clow via Phabricator via cfe-commits]

mclow.lists marked 7 inline comments as done.
mclow.lists added inline comments.


================
Comment at: libcxx/include/bit:236
+
+    if      constexpr (sizeof(_Tp) <= sizeof(unsigned int))
+        return __clz(static_cast<unsigned int>(__t))
----------------
EricWF wrote:
> Weird indentation. Clang-format please. 
The indentation is purposeful. It illustrates the structure of the code.


================
Comment at: libcxx/include/bit:240
+    else if constexpr (sizeof(_Tp) <= sizeof(unsigned long))
+        return __clz(static_cast<unsigned long>(__t))
+              - (numeric_limits<unsigned long>::digits - numeric_limits<_Tp>::digits);
----------------
EricWF wrote:
> Do we even need `if constexpr` here?
Yes. When instantiated with `_Tp == unsigned long` (say), this should be a single call to `__clz`, w/o any other code.


================
Comment at: libcxx/include/bit:251
+        while (true) {
+            __t = rotr(__t, __ulldigits);
+            if ((__iter = countl_zero(static_cast<unsigned long long>(__t))) != __ulldigits)
----------------
EricWF wrote:
> This call needs to be qualified.
I don't see why. `_Tp` must be an unsigned integral type.


================
Comment at: libcxx/include/bit:322
+    if      constexpr (sizeof(_Tp) <= sizeof(unsigned int))
+        return __popcount(static_cast<unsigned int>(__t));
+    else if constexpr (sizeof(_Tp) <= sizeof(unsigned long))
----------------
EricWF wrote:
> Do we need `if constexpr` here.
Yes. See above.


================
Comment at: libcxx/include/bit:366
+#ifndef _LIBCPP_HAS_NO_BUILTIN_IS_CONSTANT_EVALUATED
+    if (!__builtin_is_constant_evaluated ())
+        _LIBCPP_DEBUG_ASSERT( __n != numeric_limits<_Tp>::digits, "Bad input to ceil2" );
----------------
EricWF wrote:
> You don't need to guard debug assertions in C++14 constexpr functions.
Good to know; thanks.


================
Comment at: libcxx/include/bit:384
+log2p1(_Tp __t) noexcept
+{ return __t == 0 ? 0 : __bit_log2(__t) + 1; }
+
----------------
EricWF wrote:
> `__bit_log2` needs to be qualified to prevent ADL.
See above. ADL on what? There are no user-defined types here.


================
Comment at: libcxx/test/std/numerics/bit/bitops.count/countr_zero.fail.cpp:29
+{
+	(void) std::countr_zero(static_cast<int>(2));         // expected-error {{no matching function for call to 'countr_zero'}}	
+	(void) std::countr_zero(static_cast<signed int>(2));  // expected-error {{no matching function for call to 'countr_zero'}}	
----------------
EricWF wrote:
> Use `is_invocable`.
I can do that.


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

https://reviews.llvm.org/D51262