[PATCH] D51262: Implement P0553 and P0556

[Eric Fiselier via Phabricator via cfe-commits]

EricWF added a comment.
Herald added a subscriber: dexonsmith.

Please use Clang format on these changes.

Otherwise the implementation looks great, I've left some nits.



================
Comment at: include/bit:190
+template <class _Tp>
+struct __bitop_unsigned_integer
+    : public integral_constant<bool,
----------------
Use an alias template to produce this. fewer instantiations.


================
Comment at: include/bit:193
+         is_integral_v<_Tp> &&
+         is_unsigned_v<_Tp> &&
+        !is_same_v<remove_cv_t<_Tp>, bool> &&
----------------
is `bool` unsigned? Otherwise it should already be excluded.
Also are the unsigned types that aren't integral?


================
Comment at: include/bit:209
+    const unsigned int __dig = numeric_limits<_Tp>::digits;
+    return (__cnt % __dig) == 0
+        ? __t
----------------
Nit:

Use an `if` over a long ternary operator.


================
Comment at: include/bit:254
+
+    if      constexpr (sizeof(_Tp) <= sizeof(unsigned int))
+    	return __clz(static_cast<unsigned int>(__t))
----------------
Cool use of `if constexpr`.

Please clang-format these changes.


================
Comment at: include/bit:405
+
+template <class _Tp>
+inline _LIBCPP_INLINE_VISIBILITY constexpr
----------------
Please write the SFINAE using a default template parameter.

See http://libcxx.llvm.org/docs/DesignDocs/ExtendedCXX03Support.html#use-default-template-parameters-for-sfinae


================
Comment at: test/std/numerics/bit/bitops.rot/rotl.pass.cpp:106
+
+// 	{
+// 	const int dig = std::numeric_limits<__uint128_t>::digits;
----------------
Commented out code.


================
Comment at: test/std/numerics/bit/bitops.rot/rotr.pass.cpp:119
+
+// 	{
+// 	const int dig = std::numeric_limits<__uint128_t>::digits;
----------------
Please don't add commented out code.


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

https://reviews.llvm.org/D51262