jwakely added a comment. In D51262#1213514 <https://reviews.llvm.org/D51262#1213514>, @mclow.lists wrote:
> I should also mention that as a conforming extension, I have implemented the > non-numeric bit operations for `std::byte` I thought there was a DR or proposal to enable that, but I don't see one now. ================ Comment at: include/bit:405 + +template <class _Tp> +inline _LIBCPP_INLINE_VISIBILITY constexpr ---------------- mclow.lists wrote: > EricWF wrote: > > Please write the SFINAE using a default template parameter. > > > > See > > http://libcxx.llvm.org/docs/DesignDocs/ExtendedCXX03Support.html#use-default-template-parameters-for-sfinae > I think that document is misguided in cases like this; the `enable_if` on the > return type cannot be "interfered with" by the caller the way an extra > template parameter can be. > > Yes (except on constructors) libstdc++ always uses the return type instead of a default template argument list, to stop "clever" people doing `ceil2<int>(3)` and avoiding the contraint. ================ Comment at: libcxx/include/bit:378 + const unsigned __retVal = 1u << (__n + __extra); + return (_Tp) (__retVal >> __extra); + } ---------------- mclow.lists wrote: > mclow.lists wrote: > > mclow.lists wrote: > > > Quuxplusone wrote: > > > > mclow.lists wrote: > > > > > mclow.lists wrote: > > > > > > Quuxplusone wrote: > > > > > > > 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? > > > > > > Yes. I want to generate some UB here, so that this is not a "core > > > > > > constant expression" as per P1355. > > > > > Yes, the `ceil2.fail.cpp` test will not fail (for short types) if I > > > > > just return `_Tp{1} << __n;` - because of integer promotion. > > > > > > > > > Yikes. Sounds like there's some "library maintainer"ship going on here, > > > > then. Naively, I would have thought that the Working Draft had left the > > > > behavior of such constructs undefined in order to make the library > > > > vendor's life **easier** and the code **more efficient**. But you're > > > > saying that you need to pessimize the code here in order to make it > > > > "sufficiently undefined" so that its behavior at compile time will be > > > > well-defined and produce a diagnostic instead of being undefined and > > > > producing garbage? > > > > > > > > Maybe WG21 needs to invent a "really actually undefined behavior" that > > > > does not have unwanted interactions with constexpr, so that library > > > > writers can go back to generating fast clean code! > > > > > > > > Rant aside, why don't you just do `_LIBCPP_ASSERT(__n < > > > > numeric_limits<_Tp>::digits);` and leave it at that? An assertion > > > > failure isn't a compile-time constant, is it? > > > > Also, is this a kosher use of sizeof(X) * 8 as a stand-in for > > > > numeric_limits<X>::digits? > > > > > > Good catch. > > that's exactly what P1355 says. > There are three things that I'd like this code to do in the case of bad > inputs: > * Produce a diagnostic if called at constexpr time (required by P1355) > * Cause UBSAN to catch it at runtime > * Cause a libc++ assertion to go off if they are enabled. > > Note that these are all more or less independent. Those are exactly the three things that my implementation also does, which is not a coincidence because this requirement of P1355 has been discussed offline (and feedback from implementation experience sent to the author about this particular aspect of it). @Quuxplusone an assertion failure is not enabled by default, so doesn't do any good in a constant expression. There are a few different ways to make the shift undefined when the operand promotes, but you can't just avoid the complexity altogether. 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
