[PATCH] D51262: Implement P0553 and P0556

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

Quuxplusone added inline comments.


================
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);
----------------
mclow.lists wrote:
> EricWF wrote:
> > Do we even need `if constexpr` here?
> Yes. When instantiated with `_Tp == unsigned long` (say), this should be a single call to `__clz`, w/o any other code.
The optimizer should take care of that just fine. But, `if constexpr` also prevents us from unnecessarily trying to instantiate, say, `static_cast<unsigned long long>(__t)`, in the case that `__t` is of a user-defined type where that conversion's definition is ill-formed or explicitly deleted. I'm still extremely skeptical that any user-defined type is ever allowed to reach this code without UB; but if it is, then instantiating just the bare minimum of conversions might be a QoI issue. (The standard doesn't say anything about what this template should do for user-defined types, so keeping the surface area small is a good idea.)


================
Comment at: libcxx/include/bit:384
+log2p1(_Tp __t) noexcept
+{ return __t == 0 ? 0 : __bit_log2(__t) + 1; }
+
----------------
mclow.lists wrote:
> EricWF wrote:
> > `__bit_log2` needs to be qualified to prevent ADL.
> See above. ADL on what? There are no user-defined types here.
FWIW, my intuition still agrees with Eric's: adding `_VSTD::` throughout might not help anything, but if it wouldn't hurt, //and// if it stops every future code-reviewer from making the same exact comment about ADL, shouldn't you just do it? Wouldn't it save everyone some time?


================
Comment at: libcxx/test/std/numerics/bit/nothing_to_do.pass.cpp:1
+//===----------------------------------------------------------------------===//
+//
----------------
Peanut gallery says: I thought `nothing_to_do.pass.cpp` files were obsolete these days?


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

https://reviews.llvm.org/D51262