[libcxx-commits] [PATCH] D77505: [libc++] Implement <numbers>

[Louis Dionne via Phabricator via libcxx-commits]

ldionne requested changes to this revision.
ldionne added a comment.
This revision now requires changes to proceed.

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>`.



================
Comment at: libcxx/include/numbers:103
+template <class T>
+concept floating_point = std::is_floating_point_v<T>;
+
----------------
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.


================
Comment at: libcxx/test/std/numerics/numbers/illformed.fail.cpp:16
+int log2e{std::numbers::log2e_v<
+    int>}; // expected-error-re at numbers:* {{static_assert failed {{.*}} "A program that instantiates a primary template of a mathematical constant variable template is ill-formed."}}
+int log10e{std::numbers::log10e_v<int>};
----------------
Can you make this test a `.verify.cpp` instead?


================
Comment at: libcxx/test/std/numerics/numbers/specialize.pass.cpp:19
+
+constexpr float f0{std::numbers::e_v<float>};
+constexpr float f1{std::numbers::log2e_v<float>};
----------------
We should put all these tests in a `constexpr` function, and call it from a non-constexpr context and a constexpr context.


================
Comment at: libcxx/test/std/numerics/numbers/value.pass.cpp:14
+
+static_assert(std::numbers::e == 0x1.5bf0a8b145769p+1);
+static_assert(std::numbers::e_v<long double> == 0x1.5bf0a8b145769p+1l);
----------------
Same here, we should put all these as `assert`s 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).


================
Comment at: libcxx/test/std/numerics/numbers/value.pass.cpp:31
+static_assert(std::numbers::inv_pi == 0x1.45f306dc9c883p-2);
+static_assert(std::numbers::inv_pi_v<long double> == 0x1.45f306dc9c883p-2l);
+static_assert(std::numbers::inv_pi_v<float> == 0x1.45f306p-2f);
----------------
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`.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D77505/new/

https://reviews.llvm.org/D77505