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