Page MenuHomePhabricator

[libcxx] adds concept `std::uniform_random_bit_generator`
ClosedPublic

Authored by cjdb on Feb 12 2021, 12:21 AM.

Details

Summary

Implements parts of:

  • P0898R3 Standard Library Concepts
  • P1754 Rename concepts to standard_case for C++20, while we still can

Diff Detail

Event Timeline

cjdb requested review of this revision.Feb 12 2021, 12:21 AM
cjdb created this revision.
Herald added a project: Restricted Project. · View Herald TranscriptFeb 12 2021, 12:21 AM
Herald added a reviewer: Restricted Project. · View Herald Transcript
cjdb updated this revision to Diff 323234.Feb 12 2021, 12:31 AM

undoes weird clang-formatting

Thanks for working on this. It is missing uglification but besides that I only have one small nit

libcxx/include/random
1707

This should be _Ugly, maybe use _Gen?

libcxx/test/std/numerics/rand/rand.req/rand.req.urng/uniform_random_bit_generator.compile.pass.cpp
119

I think that 1 and 0 would be more obvious that ~0 and 0

miscco added inline comments.Feb 12 2021, 1:31 AM
libcxx/include/random
1704

in https://reviews.llvm.org/D93166 we have a definition of _LIBCPP_HAS_NO_CONCEPTS

Should we reuse that globally and why are there different values ofr the macro?

cjdb updated this revision to Diff 323507.Feb 12 2021, 6:52 PM
cjdb marked 2 inline comments as done.

applies @miscco's feedback

cjdb marked an inline comment as done.Feb 12 2021, 6:52 PM
cjdb added a comment.Feb 12 2021, 11:10 PM

@ldionne can we override the Mac OS format failure? What it's proposing is abominable.

cjdb updated this revision to Diff 323528.Feb 12 2021, 11:14 PM

undoes !_LIBCPP_HAS_NO_CONCEPTS change in an attempt to get the Mac OS build working

undoes !_LIBCPP_HAS_NO_CONCEPTS change in an attempt to get the Mac OS build working

Yes there seems to be a bug in that code. I've put it in a block for GCC specific code. (This is not obvious due to the lack of indention of the section.)
I'm testing a fix.

libcxx/include/random
22

Please add comment this has been added in C++20.

libcxx/test/std/numerics/rand/rand.req/rand.req.urng/uniform_random_bit_generator.compile.pass.cpp
21

Can you guard this with #ifndef _LIBCPP_HAS_NO_INT128?

123

Can you add a test where min() and max() return the same value?

cjdb updated this revision to Diff 323584.Feb 13 2021, 6:22 PM
cjdb marked 2 inline comments as done.

applies @Mordante's feedback

cjdb added a comment.Feb 13 2021, 6:22 PM

Yes there seems to be a bug in that code. I've put it in a block for GCC specific code. (This is not obvious due to the lack of indention of the section.)

Oh, it might be because Clang's concepts are incomplete and so the feature-test macro's value is too high for us? Anyway, if you update everything in one fell swoop, I think that'd be best :-)

Yes there seems to be a bug in that code. I've put it in a block for GCC specific code. (This is not obvious due to the lack of indention of the section.)

Oh, it might be because Clang's concepts are incomplete and so the feature-test macro's value is too high for us? Anyway, if you update everything in one fell swoop, I think that'd be best :-)

No it was the placement in the file. But I'll fix <random> and <concept> after the fix has landed.

Mordante added inline comments.Feb 14 2021, 6:28 AM
libcxx/test/std/numerics/rand/rand.req/rand.req.urng/uniform_random_bit_generator.compile.pass.cpp
122

Is this a copy-paste error, I assume this should be zero.

cjdb updated this revision to Diff 324166.Feb 16 2021, 6:59 PM

fixes copypasta

ldionne accepted this revision.Feb 17 2021, 2:28 PM

LGTM apart for the nitpick. Let's settle on what we do about it and ship this.

libcxx/include/random
1704

why are there different values ofr the macro?

I don't know. @cjdb do you know what 201907L vs 201811L represents for __cpp_concepts?

Should we reuse that globally

Depending to the answer to the question above, I think it would make sense to use #if _LIBCPP_STD_VER > 17 && !defined(_LIBCPP_HAS_NO_CONCEPTS).

This revision is now accepted and ready to land.Feb 17 2021, 2:28 PM
cjdb added inline comments.Feb 17 2021, 2:42 PM
libcxx/include/random
1704

I believe @Mordante has a patch to fix this globally. As for 201811L vs 201907L, this is consistent with what's in libc++ <concepts> right now.

cjdb added a subscriber: saar.raz.Feb 17 2021, 3:38 PM
cjdb added inline comments.
libcxx/include/random
1704

@saar.raz do you know the difference between these two values for __cpp_concepts?

ldionne added inline comments.Feb 18 2021, 9:19 AM
libcxx/include/random
1704

Oh, thanks for the link. @cjdb let's use 201907L consistently everywhere if you're OK with that.

This revision was automatically updated to reflect the committed changes.
Mordante added inline comments.Feb 18 2021, 10:37 PM
libcxx/include/random
1704

I used 201907L since that's the version used in Clang since @saar.raz's branch has been merged.