[PATCH] D51262: Implement P0553 and P0556

[Eric Fiselier via Phabricator via cfe-commits]

EricWF added a comment.

I would like to see a version of this after the inline comments are addressed.



================
Comment at: libcxx/include/bit:193
+         is_unsigned_v<_Tp> &&
+        !is_same_v<remove_cv_t<_Tp>, bool> &&
+        !is_same_v<remove_cv_t<_Tp>, char> &&
----------------
`_IsNotSame` is faster and better to use here.

Also please put `_LIBCPP_NODEBUG_TYPE` on this alias. Otherwise it could generate unwanted debug info.


================
Comment at: libcxx/include/bit:203
+template<class _Tp>
+inline _LIBCPP_INLINE_VISIBILITY constexpr
+enable_if_t<__bitop_unsigned_integer<_Tp>::value, _Tp>
----------------
Why the explicit inline hint?


================
Comment at: libcxx/include/bit:208
+    const unsigned int __dig = numeric_limits<_Tp>::digits;
+    return (__cnt % __dig) == 0
+        ? __t
----------------
Why did you choose to use the ternary operator here, but you use an `if` in `rotr`?


================
Comment at: libcxx/include/bit:236
+
+    if      constexpr (sizeof(_Tp) <= sizeof(unsigned int))
+        return __clz(static_cast<unsigned int>(__t))
----------------
Weird indentation. Clang-format please. 


================
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);
----------------
Do we even need `if constexpr` here?


================
Comment at: libcxx/include/bit:251
+        while (true) {
+            __t = rotr(__t, __ulldigits);
+            if ((__iter = countl_zero(static_cast<unsigned long long>(__t))) != __ulldigits)
----------------
This call needs to be qualified.


================
Comment at: libcxx/include/bit:268
+    return __t != numeric_limits<_Tp>::max()
+        ? countl_zero(static_cast<_Tp>(~__t))
+        : numeric_limits<_Tp>::digits;
----------------
This should be qualified.


================
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))
----------------
Do we need `if constexpr` here.


================
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" );
----------------
You don't need to guard debug assertions in C++14 constexpr functions.


================
Comment at: libcxx/include/bit:384
+log2p1(_Tp __t) noexcept
+{ return __t == 0 ? 0 : __bit_log2(__t) + 1; }
+
----------------
`__bit_log2` needs to be qualified to prevent ADL.


================
Comment at: libcxx/test/std/numerics/bit/bit.pow.two/ceil2.fail.cpp:36
+{
+	(void) std::ceil2(static_cast<int>(2));         // expected-error {{no matching function for call to 'ceil2'}}
+	(void) std::ceil2(static_cast<signed int>(2));  // expected-error {{no matching function for call to 'ceil2'}}
----------------
This can be written as a `.pass.cpp` test that uses `is_invocable`.


================
Comment at: libcxx/test/std/numerics/bit/bit.pow.two/floor2.fail.cpp:30
+	(void) std::floor2(static_cast<int>(2));         // expected-error {{no matching function for call to 'floor2'}}	
+	(void) std::floor2(static_cast<signed int>(2));  // expected-error {{no matching function for call to 'floor2'}}	
+	(void) std::floor2(static_cast<long>(2));        // expected-error {{no matching function for call to 'floor2'}}	
----------------
Use `is_invocable`.


================
Comment at: libcxx/test/std/numerics/bit/bit.pow.two/ispow2.fail.cpp:29
+{
+	(void) std::ispow2(static_cast<int>(2));         // expected-error {{no matching function for call to 'ispow2'}}	
+	(void) std::ispow2(static_cast<signed int>(2));  // expected-error {{no matching function for call to 'ispow2'}}	
----------------
Use `is_invocable`.


================
Comment at: libcxx/test/std/numerics/bit/bitops.count/countl_one.fail.cpp:30
+	(void) std::countl_one(static_cast<int>(2));         // expected-error {{no matching function for call to 'countl_one'}}	
+	(void) std::countl_one(static_cast<signed int>(2));  // expected-error {{no matching function for call to 'countl_one'}}	
+	(void) std::countl_one(static_cast<long>(2));        // expected-error {{no matching function for call to 'countl_one'}}	
----------------
Use `is_invocable`.


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


================
Comment at: libcxx/test/std/numerics/bit/bitops.count/countr_one.fail.cpp:30
+	(void) std::countr_one(static_cast<int>(2));         // expected-error {{no matching function for call to 'countr_one'}}	
+	(void) std::countr_one(static_cast<signed int>(2));  // expected-error {{no matching function for call to 'countr_one'}}	
+	(void) std::countr_one(static_cast<long>(2));        // expected-error {{no matching function for call to 'countr_one'}}	
----------------
Use `is_invocable`.


================
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'}}	
----------------
Use `is_invocable`.


================
Comment at: libcxx/test/std/numerics/bit/bitops.count/popcount.fail.cpp:30
+	(void) std::popcount(static_cast<int>(2));         // expected-error {{no matching function for call to 'popcount'}}	
+	(void) std::popcount(static_cast<signed int>(2));  // expected-error {{no matching function for call to 'popcount'}}	
+	(void) std::popcount(static_cast<long>(2));        // expected-error {{no matching function for call to 'popcount'}}	
----------------
Use `is_invocable`.


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

https://reviews.llvm.org/D51262