mclow.lists marked 4 inline comments as done.
mclow.lists added inline comments.
================
Comment at: libcxx/include/bit:252
+ while (true) {
+ __t = rotr<_Tp>(__t, __ulldigits);
+ if ((__iter = countl_zero(static_cast<unsigned long long>(__t)))
!= __ulldigits)
----------------
Quuxplusone wrote:
> Since `__t` is already of type `_Tp`, the explicit template argument `<_Tp>`
> is unnecessary here.
> Also, is the ADL here intentional?
Right about the `<_Tp>`
================
Comment at: libcxx/include/bit:253
+ __t = rotr<_Tp>(__t, __ulldigits);
+ if ((__iter = countl_zero(static_cast<unsigned long long>(__t)))
!= __ulldigits)
+ break;
----------------
Quuxplusone wrote:
> I forget the rules of name hiding; is this `countl_zero` also an ADL call?
> (My experimentation implies it's not? but it would still be clearer to use
> `_VSTD::countl_zero` if that's what you mean.)
>
> As a general rule, I strongly recommend writing side-effects //outside// of
> `if` conditions; like, on the previous line or something; unless writing it
> all on a single line has some concrete benefit.
Not an ADL call, because `_Tp` is always a built-in type.
================
Comment at: libcxx/include/bit:365
+ if (__t < 2) return 1;
+ const unsigned __n = numeric_limits<_Tp>::digits - countl_zero((_Tp)(__t -
1u));
+
----------------
Quuxplusone wrote:
> Incidentally, why `(_Tp)(__t - 1u)` instead of the formula `__t - _Tp{1}`
> used elsewhere in this header?
Because I want to end up with a value of type `_Tp`, and (for short types),
`__t - _Tp{1}` doesn't get me that. (Integer promotion). `((char) 23) -
((char) 1)` is not type `char`
================
Comment at: libcxx/include/bit:378
+ const unsigned __retVal = 1u << (__n + __extra);
+ return (_Tp) (__retVal >> __extra);
+ }
----------------
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.
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
