Quuxplusone added inline comments.
================
Comment at: libcxx/include/bit:199
+ !is_same_v<remove_cv_t<_Tp>, char32_t>
+ > {};
+
----------------
Given how heavily the code controlled by this trait depends on
`numeric_limits<_Tp>`, would it make sense to add something in here about how
that specialization should exist?
I agree with @EricWF's earlier comment: I can't think of any types that would
satisfy `(!is_integral_v<_Tp>) && is_unsigned_v<_Tp>`. (But just TIL that the
signedness of `wchar_t` is implementation-defined.)
================
Comment at: libcxx/include/bit:211
+ ? __t
+ : (__t << (__cnt % __dig)) | (__t >> (__dig - (__cnt % __dig)));
+}
----------------
No sane compiler would actually generate three `mod` operations for the three
instances of repeated subexpression `__cnt % __dig`, would they?
================
Comment at: libcxx/include/bit:252
+ while (true) {
+ __t = rotr<_Tp>(__t, __ulldigits);
+ if ((__iter = countl_zero(static_cast<unsigned long long>(__t)))
!= __ulldigits)
----------------
Since `__t` is already of type `_Tp`, the explicit template argument `<_Tp>` is
unnecessary here.
Also, is the ADL here intentional?
================
Comment at: libcxx/include/bit:253
+ __t = rotr<_Tp>(__t, __ulldigits);
+ if ((__iter = countl_zero(static_cast<unsigned long long>(__t)))
!= __ulldigits)
+ break;
----------------
I forget the rules of name hiding; is this `countl_zero` also an ADL call? (My
experimentation implies it's not? but it would still be clearer to use
`_VSTD::countl_zero` if that's what you mean.)
As a general rule, I strongly recommend writing side-effects //outside// of
`if` conditions; like, on the previous line or something; unless writing it all
on a single line has some concrete benefit.
================
Comment at: libcxx/include/bit:255
+ break;
+ __ret += __iter;
+ }
----------------
clang-format?
================
Comment at: libcxx/include/bit:331
+ int __ret = 0;
+ while (__t != 0) // not a fixed loop here b/c we can exit early
+ {
----------------
FWIW, this comment seems tautological.
================
Comment at: libcxx/include/bit:365
+ if (__t < 2) return 1;
+ const unsigned __n = numeric_limits<_Tp>::digits - countl_zero((_Tp)(__t -
1u));
+
----------------
Incidentally, why `(_Tp)(__t - 1u)` instead of the formula `__t - _Tp{1}` used
elsewhere in this header?
================
Comment at: libcxx/include/bit:378
+ const unsigned __retVal = 1u << (__n + __extra);
+ return (_Tp) (__retVal >> __extra);
+ }
----------------
Why so complicated? Is there a unit test that demonstrates why you can't just
use `return _Tp{1} << __n;` in this case as well?
Also, is this a kosher use of `sizeof(X) * 8` as a stand-in for
`numeric_limits<X>::digits`?
Also, speaking of unit tests, I don't see any unit tests for e.g.
`std::ceil2(256)` or `std::ceil2(65536)`. Shouldn't there be some?
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
