[PATCH] D51262: Implement P0553 and P0556

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

Quuxplusone added inline comments.


================
Comment at: libcxx/include/bit:378
+    	const unsigned __retVal = 1u << (__n + __extra);
+    	return (_Tp) (__retVal >> __extra);
+    }
----------------
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?


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

https://reviews.llvm.org/D51262