[PATCH] D51262: Implement P0553 and P0556

Jonathan Wakely via Phabricator via cfe-commits cfe-commits at lists.llvm.org
Tue Jun 25 02:26:36 PDT 2019


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





More information about the cfe-commits mailing list