mclow.lists marked an inline comment as done.
mclow.lists 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:
> > > 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.
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
