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