[libcxx-commits] [PATCH] D77505: [libc++] Implement <numbers>
Louis Dionne via Phabricator via libcxx-commits
libcxx-commits at lists.llvm.org
Tue Jun 9 08:46:07 PDT 2020
ldionne requested changes to this revision.
ldionne added inline comments.
This revision now requires changes to proceed.
================
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>};
----------------
ldionne wrote:
> We should put all these tests in a `constexpr` function, and call it from a non-constexpr context and a constexpr context.
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.
================
Comment at: libcxx/test/std/numerics/numbers/specialize.pass.cpp:15
+#if defined(__clang__)
+#pragma clang diagnostic ignored "-Wunused-variable"
+#endif
----------------
Indentation:
```
#if ...
# pragma ...
#endif
```
================
Comment at: libcxx/test/std/numerics/numbers/specialize.pass.cpp:80
+
+constexpr bool constant{test()};
+
----------------
Use `static_assert` here please, it's what we usually do.
================
Comment at: libcxx/test/std/numerics/numbers/value.pass.cpp:84
+
+[[maybe_unused]] constexpr bool constant{test()};
+
----------------
`static_assert`
================
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);
----------------
ldionne wrote:
> 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).
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.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D77505/new/
https://reviews.llvm.org/D77505
More information about the libcxx-commits
mailing list