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

[Louis Dionne via Phabricator via libcxx-commits]

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