[PATCH] D51262: Implement P0553 and P0556

[Arthur O'Dwyer via Phabricator via cfe-commits]

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