Page MenuHomePhabricator

[libc++] Implement <numbers>
ClosedPublic

Authored by tambre on Apr 5 2020, 9:58 AM.

Details

Reviewers
ldionne
EricWF
zoecarver
curdeius
Group Reviewers
Restricted Project
Commits
rG4f6c4b473c4a: [libc++] Implement <numbers>
Summary

Constants have 33 significant decimal digits for IEEE 754 128-bit floating-point numbers.

Diff Detail

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
Herald added a project: Restricted Project. · View Herald TranscriptApr 5 2020, 9:58 AM
Herald added a reviewer: Restricted Project. · View Herald Transcript

Is docs/TestingLibcxx.rst up-to-date? I can't find the lit.site.cfg for libc++ in my build directory to be able to run tests locally. I do see others for libc++ benchmarks and LLVM components.

tambre updated this revision to Diff 255285.Apr 6 2020, 4:27 AM

Add version macro

tambre marked an inline comment as done.Apr 6 2020, 4:29 AM
tambre added inline comments.
libcxx/utils/generate_feature_test_macro_components.py
592

Minor fix for Python 3.

rsmith added a subscriber: rsmith.Apr 6 2020, 2:17 PM

Should we have some kind of test coverage that the values of these constants are correct?

libcxx/test/std/numerics/numbers/non_floating_point.fail.cpp
13–25 ↗(On Diff #255285)

These are all missing their template arguments. I'm not sure exactly what this test is checking for, but I'm assuming it's not that these result in parse errors. (Should these tests omit the _v to check for a narrowing conversion, or should they include the <int> to check the floating-point constraint?)

libcxx/test/std/numerics/numbers/user_type.pass.cpp
19–31

These are similarly missing their template arguments.

tambre added a comment.Apr 7 2020, 2:11 AM

Should we have some kind of test coverage that the values of these constants are correct?

How would we test constants?

Regarding tests, as I mentioned in my comment, I couldn't figure out how to run libc++ tests locally, so I wrote them blindly. Maybe I missed something, I'll try again tonight.

tambre updated this revision to Diff 255700.Apr 7 2020, 9:14 AM
tambre marked 4 inline comments as done.

Fix floating point constraint tests, remove bad test

libcxx/test/std/numerics/numbers/non_floating_point.fail.cpp
13–25 ↗(On Diff #255285)

This was meant to test "Pursuant to [namespace.std], a program may partially or explicitly specialize a mathematical constant variable template provided that the specialization depends on a program-defined type."
I've removed this test for now, as I'm not sure how to properly test this and if there's even a point.

libcxx/test/std/numerics/numbers/user_type.pass.cpp
19–31

Fixed. Meant to check floating point constraint.

ldionne: ping

EricWF requested changes to this revision.Apr 16 2020, 9:13 AM
EricWF added a subscriber: EricWF.

This needs a bunch of additional tests. Specifically of the passing kind :-)

This revision now requires changes to proceed.Apr 16 2020, 9:13 AM
tambre updated this revision to Diff 261687.May 3 2020, 3:05 AM

Add more tests, assert on ill-formed specialization

tambre added a comment.May 3 2020, 3:07 AM

This needs a bunch of additional tests. Specifically of the passing kind :-)

Added a few more tests.
I have no idea what else I could test, so let me know. :)

Also added a static_assert for ill-formed programs.

EricWF: ping

thanks @curdeius for pointing this.
Hi @tambre
I was unaware of this work and ended of doing same work.
please take things from here D78427 which seems reasonable.
and update the this review. so i can cancel my review.

thanks @curdeius for pointing this.
Hi @tambre
I was unaware of this work and ended of doing same work.
please take things from here D78427 which seems reasonable.
and update the this review. so i can cancel my review.

Unfortunate to have a bit of duplicated work.
I'll take a look at yours and borrow what's useful. :)

tambre updated this revision to Diff 265837.May 23 2020, 12:36 AM

Borrow useful things from D78427:

  • A test
  • Version guard
  • Double include test

Rebased on master.

Adding a few reviewers from D78427 so they can take a look at this too.

Borrow useful things from D78427:

  • A test
  • Version guard
  • Double include test

Rebased on master.

you can also take comments like
https://oeis.org/A001113 for e.

Borrow useful things from D78427:

  • A test
  • Version guard
  • Double include test

Rebased on master.

you can also take comments like
https://oeis.org/A001113 for e.

I didn't add those, as I don't think it'd be appropriate to link to an external site in standard library headers.

curdeius added inline comments.May 28 2020, 3:02 PM
libcxx/test/std/numerics/numbers/specialize.pass.cpp
15

Typo: instantiations.

tambre updated this revision to Diff 267278.May 29 2020, 10:01 AM
tambre marked 2 inline comments as done.

Fix typo in a comment.

curdeius accepted this revision.May 30 2020, 4:57 AM

LGTM. But you need to wait for a review from somebody from libc++ group.

ldionne requested changes to this revision.Jun 2 2020, 8:14 AM

Thanks for the contribution, and thanks to other reviewers for taking a look before!

To run the tests, perform the CMake configuration step, then run ninja -C <build-dir> check-cxx-deps, and then <build-dir>/bin/llvm-lit -sv libcxx/test/std/<whatever>.

libcxx/include/numbers
104

I don't see that concept defined inside <numbers> in http://wg21.link/p0631 -- should this be kept as an implementation detail instead?

Or was it moved there by another paper? In all cases, if that is public according to the Standard, please add tests for it.

libcxx/test/std/numerics/numbers/illformed.fail.cpp
16 ↗(On Diff #267278)

Can you make this test a .verify.cpp instead?

libcxx/test/std/numerics/numbers/specialize.pass.cpp
20

We should put all these tests in a constexpr function, and call it from a non-constexpr context and a constexpr context.

libcxx/test/std/numerics/numbers/value.pass.cpp
15

Same here, we should put all these as asserts inside a constexpr function, and call it from a constexpr context but also a runtime context. Otherwise, we're only testing the constexpr version, which doesn't take the same code path inside the compiler (no codegen, etc).

32

Please also check the specialization of foo_v<double> explicitly. Other implementations could conceivably not rely on that specialization to implement e.g. inv_pi.

This revision now requires changes to proceed.Jun 2 2020, 8:14 AM
tambre updated this revision to Diff 269009.Jun 6 2020, 4:03 AM
tambre marked 6 inline comments as done.

Address comments.

tambre added a comment.Jun 6 2020, 4:04 AM

I've addressed the comments.

libcxx/include/numbers
104

Yes, this is an implementation detail. I've prefixed it with two underscores now.

Technically this should be std::floating_point from the concepts library, but libc++ doesn't have a full implementation of that yet. Besides, it seems a bit heavy to pull in <concepts> just for one thing, when it's simple enough to define here.

ldionne requested changes to this revision.Jun 9 2020, 8:23 AM
ldionne added inline comments.
libcxx/test/std/numerics/numbers/specialize.pass.cpp
16

Indentation:

#if ...
#   pragma ...
#endif
20

I don't feel like you really addressed my comment -- my point was:

constexpr bool tests()
{
    float f0{std::numbers::e_v<float>};
    ...
}

Then, call that from a constexpr and a non-constexpr context.

Furthermore, you should probably try taking the address of these things to make sure they are defined somewhere.

81

Use static_assert here please, it's what we usually do.

libcxx/test/std/numerics/numbers/value.pass.cpp
15

My point was:

constexpr bool tests()
{
    assert(std::numbers::e == ...);
    ...
}

Otherwise you're still only using these constants from a constexpr context, i.e. the static_assert they appear in.

85

static_assert

This revision now requires changes to proceed.Jun 9 2020, 8:23 AM
tambre updated this revision to Diff 269937.Jun 10 2020, 12:11 PM
tambre marked 5 inline comments as done.

Address comments.
Add test checking if the numbers are defined.

tambre marked an inline comment as done.Jun 10 2020, 12:13 PM

Hopefully I've managed to address the comments correctly this time around.

libcxx/test/std/numerics/numbers/specialize.pass.cpp
16

Unfortunately clang-format doesn't agree and will try to revert the indentation. Fixed manually.

ldionne accepted this revision.Jun 16 2020, 10:55 AM
ldionne added inline comments.
libcxx/test/std/numerics/numbers/specialize.pass.cpp
16

We don't use clang-format (yet) in libc++.

Please commit this for me in case no one else has objections. I lack commit privileges.

kamleshbhalui added a comment.EditedJun 19 2020, 12:09 AM

Please commit this for me in case no one else has objections. I lack commit privileges.

name and email?

Please commit this for me in case no one else has objections. I lack commit privileges.

name and email?

Raul Tambre <raul@tambre.ee>

This revision was not accepted when it landed; it landed in state Needs Review.Jun 19 2020, 2:07 AM
Closed by commit rG4f6c4b473c4a: [libc++] Implement <numbers> (authored by tambre, committed by kamleshbhalui). · Explain Why
This revision was automatically updated to reflect the committed changes.

@tambre What compiler(s) did you test this with? It's breaking pretty much all of the bots because of unguarded concepts use (I think -- haven't looked thoroughly yet).

@tambre What compiler(s) did you test this with? It's breaking pretty much all of the bots because of unguarded concepts use (I think -- haven't looked thoroughly yet).

Sorry, I had not seen https://reviews.llvm.org/D82171 yet. Thanks for being on top of this.

@tambre What compiler(s) did you test this with? It's breaking pretty much all of the bots because of unguarded concepts use (I think -- haven't looked thoroughly yet).

I tested only with Clang. I'll make sure to test with GCC too in the future.

Thanks for fixing up D82171 while I was away.