This is an archive of the discontinued LLVM Phabricator instance.

[libc++] Fix __bitop_unsigned_integer, rename to __is_unsigned_integer, and fix constexprness of std::countr_zero(__uint128_t)
ClosedPublic

Authored by Quuxplusone on May 12 2021, 7:23 AM.

Details

Summary

Fixes https://bugs.llvm.org/show_bug.cgi?id=49450

There are only five unsigned integer types, so we should just list them out.
Also provide `__is_signed_integer`, even though the Standard doesn't consume
that trait anywhere yet.

Notice that `concept uniform_random_bit_generator` is specifically specified
to rely on `concept unsigned_integral` and *not* `__is_unsigned_integer`.
Instantiating `std::ranges::sample` with a type `U` satisfying
`uniform_random_bit_generator` where `unsigned_integral<U::result_type>`
and not `__is_unsigned_integer<U::result_type>` is simply IFNDR.

Also vastly improve test coverage for the <bit> manipulation functions, using our modern "test runtime and constexpr-time with the same function" approach.

Diff Detail

Event Timeline

jwakely requested review of this revision.May 12 2021, 7:23 AM
jwakely created this revision.
Herald added a project: Restricted Project. · View Herald TranscriptMay 12 2021, 7:23 AM
Herald added a reviewer: Restricted Project. · View Herald Transcript
Quuxplusone requested changes to this revision.May 12 2021, 7:49 AM
Quuxplusone added a subscriber: Quuxplusone.
Quuxplusone added inline comments.
libcxx/include/bit
89

Seems like we should add char8_t while we're here, yeah? (It is also integral and unsigned but not an unsigned integer type.) For bonus clarity, we could make a type trait __is_integer_type<_Tp> and then this whole constraint on line 97 would be simply

static_assert(__is_integer_type<_Tp>::value && is_unsigned<_Tp>::value, "__rotl requires an unsigned integer type");

...except that a static_assert is not a "constraint," so we're just totally nonconforming here.
@jwakely, thoughts? I'd volunteer to commandeer if you like.

This revision now requires changes to proceed.May 12 2021, 7:49 AM
jwakely added inline comments.May 12 2021, 8:29 AM
libcxx/include/bit
89

Yes, that should be there too. Feel free to take this, I only submitted it here because it seemed better to put the patch here than in bugzilla.

Quuxplusone commandeered this revision.May 12 2021, 9:39 AM
Quuxplusone edited reviewers, added: jwakely; removed: Quuxplusone.
libcxx/include/bit
89

Blech.
@mclow.lists, @ldionne: libc++ currently supports __uint128_t for these functions; do we want to keep supporting it?
libstdc++ does not support it: https://godbolt.org/z/jK9q8z6oT

I'm going to operate on the assumption that we want to keep it (as a non-conforming extension)...

jwakely added inline comments.May 12 2021, 10:35 AM
libcxx/include/bit
89

If you want non-conforming extensions then you need to compile with -std=gnu++20 not -std=c++20 and if you do that, libstdc++ does support unsigned __int128 here.

jwakely added inline comments.May 12 2021, 10:39 AM
libcxx/include/bit
89

The static assert is not a problem, because you're only looking at the internal __rotl function which doesn't need to be constrained. The public rotl function is constrained correctly, using enable_if.

Quuxplusone retitled this revision from [libc++] Fix constraint for [bit.pow.two] functions to [libc++] Fix __bitop_unsigned_integer, rename to __is_unsigned_integer, and fix constexprness of std::countr_zero(__uint128_t).
Quuxplusone edited the summary of this revision. (Show Details)
libcxx/include/bit
97

I relaxed the static_assert on the internal functions for the same (bad) intuitive reason that the internal functions are provided in the first place. We never actually use any of these internal functions, but if we did, it might be nice to be able to use them on, I dunno, char or something. I'm highly ambivalent about whether we should be asserting anything at all, or if we do, what it should be. So I'll take suggestions if anyone's got any.

136–142

This is a bugfix: std::countr_zero(__uint128_t(1) << 64) had been trying to evaluate __libcpp_ctz(0uLL) in constexpr context, and that's not allowed. The old tests didn't catch it but the new tests do.

FWIW

libcxx/include/bit
97

FWIW libstdc++ has similar internal versions of these functions, available from C++14 up, and I didn't constrain them or add assertions. The public functions are responsible for input validation (if needed) and the internal functions assume the caller knows what they're doing.

Not sure what happened with https://buildkite.com/llvm-project/libcxx-ci/builds/3210 . Rebase and poke buildkite; let's see if buildkite fixed itself.

Okay, what if we don't rearrange the order in which <bit> includes <__debug>?
(I'm preparing another patch to just alphabetize all the includes everywhere.)

Well, I don't know what's throwing bad_cast from inside the regex code, so let's just poke buildkite with something totally innocuous for now and see what happens.
https://buildkite.com/llvm-project/libcxx-ci/builds/3225

Okay, 92260d7a186425510e96b7036b467a6889d08d97 reverted the thing that was probably breaking all the tests. Let's try buildkite again.

Okay, what if we don't rearrange the order in which <bit> includes <__debug>?
(I'm preparing another patch to just alphabetize all the includes everywhere.)

clang-format has

SortIncludes, IncludeCategories, and IncludeBlocks
Mordante accepted this revision as: Mordante.May 17 2021, 10:05 AM
Mordante added a subscriber: Mordante.

LGTM, with a few suggestions.

libcxx/include/type_traits
816 ↗(On Diff #345131)

How do you feel about using __libcpp_is_signed_integer to match the __libcpp_is_floating_point below?

libcxx/test/std/numerics/bit/bit.pow.two/bit_ceil.pass.cpp
60 ↗(On Diff #345131)

What about removing this function and add it to the generic test and use it in a if constexpr(std::same_as<T, __uint128_t>) block?
(Obviously still guarded by`#ifndef _LIBCPP_HAS_NO_INT128`)

libcxx/include/type_traits
816 ↗(On Diff #345131)

I wasn't sure what the convention is here. It looks like we can't use e.g. __is_floating_point because that's sometimes a builtin, so we use __libcpp_is_floating_point instead? In this case __is_signed_integer isn't a builtin — well, isn't one today — but yeah, I guess on this second glance I am coming around to naming this entrypoint with a leading __libcpp_. That way, if it ever becomes a builtin, we won't have to change its call-sites.

libcxx/test/std/numerics/bit/bit.pow.two/bit_ceil.pass.cpp
60 ↗(On Diff #345131)

Part of my thinking here was that these tests could be ported over into libcxx/test/libcxx/ to test std::__bit_ceil and so on, in pre-'17 language modes, so we want to keep avoiding if constexpr.
But clearly I didn't actually do that porting-over; and I did remove the trailing ,"" from all the static_asserts; so I guess it'd be okay to move this into test(). It would make the file a bit shorter. Will do, or at least consider.

Take @Mordante's review comments; poke buildbot with some header-alphabetization NFC too.

This revision was not accepted when it landed; it landed in state Needs Review.May 18 2021, 4:57 PM
This revision was automatically updated to reflect the committed changes.