Page MenuHomePhabricator

[libcxx] Add c++20 <numbers>
AbandonedPublic

Authored by kamleshbhalui on Apr 18 2020, 7:09 AM.

Details

Reviewers
EricWF
ldionne
mclow.lists
zoecarver
jfb
Group Reviewers
Restricted Project
Summary

Diff Detail

Event Timeline

kamleshbhalui created this revision.Apr 18 2020, 7:09 AM
kamleshbhalui edited the summary of this revision. (Show Details)Apr 18 2020, 7:09 AM
kamleshbhalui removed a reviewer: Restricted Project.Apr 18 2020, 6:00 PM

Cleanup and
Add one more test.

kamleshbhalui added a reviewer: Restricted Project.Apr 18 2020, 9:23 PM
kamleshbhalui edited the summary of this revision. (Show Details)Apr 18 2020, 9:55 PM
kamleshbhalui added a reviewer: mclow.lists.

Thanks for implementing this.

Please update cxx2a_status.html.

libcxx/include/numbers
81

This needs to be mangled (__value).

85

Why not static_assert(false)? Wouldn't that be more clear?

libcxx/test/std/numerics/numbers/numbers.fail.cpp
19

What's this testing?

kamleshbhalui marked an inline comment as done.Apr 21 2020, 5:54 PM
kamleshbhalui added inline comments.
libcxx/include/numbers
85

in that case static_assert will fail even when this template is not instantiated.

kamleshbhalui marked an inline comment as done.Apr 21 2020, 5:58 PM
kamleshbhalui added inline comments.
libcxx/test/std/numerics/numbers/numbers.fail.cpp
19

this testing is that static assert should fail when other than floating type is used.

kamleshbhalui added a reviewer: zoecarver.

Thanks for review.
added cxx2a _statu.html

changed value to __value

Of course, thanks for working on this. LGTM.

(you'll still need a libc++ maintainer's review)

libcxx/include/numbers
85

You're right! I always forget :)

curdeius added inline comments.
libcxx/include/numbers
80

Given that it's a C++20 addition, I think that you don't need to use _LIBCPP_CONSTEXPR and directly write constexpr. This remarks concerns other usages below.

81

It would be more readable to call it __false_value or alike, because __value seems too generic.

85

Why Using Capital Letters?

What about unifying a bit static assert messages to match other in libc++?

Examples:

static_assert(is_trivially_copyable<_Tp>::value, "std::atomic<Tp> requires that 'Tp' be a trivially copyable type");
static_assert(__is_cpp17_move_insertable<allocator_type>::value, "The specified type does not meet the requirements of Cpp17MoveInsertible");
static_assert(__bitop_unsigned_integer<_Tp>::value, "__rotl requires unsigned");

Maybe something like "<numbers> header requires a floating-point type" or mentioning the corresponding concept (floating_point)?

libcxx/utils/generate_feature_test_macro_components.py
596

It should be written as int(201907), otherwise it won't work on Python3.

Thanks for the review @curdeius.
addressed your concerns.

Thanks for the fast update!

libcxx/include/numbers
194

Where did you take all the values? Do you have a link to e.g. a math notebook online where you computed all those?

libcxx/test/std/numerics/numbers/numbers.pass.cpp
77

Have you clang-formatted your patch?

Thanks for review @curdeius
clang-formatted the diff.
and used http://mpmath.org/ for calculating all constants.

jfb added a subscriber: scanon.May 6 2020, 10:59 AM
curdeius added inline comments.May 6 2020, 11:43 AM
libcxx/test/std/language.support/support.limits/support.limits.general/version.version.pass.cpp
113

It seems that you formatted a bit too much. I think that you should avoid formatting auto-generated files.

curdeius added inline comments.May 6 2020, 11:59 AM
libcxx/include/numbers
186

Typo: gamma

curdeius added inline comments.May 6 2020, 12:28 PM
libcxx/include/numbers
189

For the verification of the numbers, I'd put links to each sequence, e.g. https://oeis.org/A001620 for gamma.
BTW, it seems to be a difference on the last 5 digits. It seems that the sequence 240240243 has incorrectly repeated 240.

Incorporated @curdeius suggestion.

I've just realized that there is already D77505. You should probably think about helping to incorporate your changes into this other PR and abandon this one.

kamleshbhalui abandoned this revision.May 22 2020, 8:16 AM

duplicate of D77505