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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
