[PATCH] D51262: Implement P0553 and P0556

[Eric Fiselier via Phabricator via cfe-commits]

EricWF accepted this revision.
EricWF added inline comments.
This revision is now accepted and ready to land.


================
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);
----------------
Quuxplusone wrote:
> 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.)
@mclow.lists That's what already happens https://godbolt.org/z/5JX3md

Clang doesn't emit the dead branch at any optimization level. So the `if constexpr` isn't needed.
But the extra tokens make it read like it is needed, which as you argued for `_VSTD`, is confusing.


================
Comment at: libcxx/include/bit:366
+
+#ifndef _LIBCPP_HAS_NO_BUILTIN_IS_CONSTANT_EVALUATED
+    if (!__builtin_is_constant_evaluated ())
----------------
I don't want us to get in the habit of conditionally compiling based on `__builtin_is_constant_evaluated()`.

I think the following formulation would be cleaner.
```
// Somewhere in <type_traits>
#if _LIBCPP_HAS_NO_BUILTIN_IS_CONSTANT_EVALUATED
constexpr bool __libcpp_is_constant_evaluated() { return false; }
#else
constexpr bool __libcpp_is_constant_evaluated() { return __builtin_is_constant_evaluated(); }
#endif

// in ceil2

_LIBCPP_ASSERT(__libcpp_is_constant_evaluated() || <cond>);


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

https://reviews.llvm.org/D51262