[libcxx-commits] [PATCH] D79555: [libc++] [C++20] [P0415] Constexpr for std::complex.

[Louis Dionne via Phabricator via libcxx-commits]

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

Sorry for sitting on this for so long. Let's push this through now.



================
Comment at: libcxx/include/complex:585
 
+inline _LIBCPP_INLINE_VISIBILITY _LIBCPP_CONSTEXPR_AFTER_CXX17 float
+__copysign_constexpr(float __lcpp_x, float __lcpp_y) {
----------------
We already jump through some hoops in `<math.h>` to provide `copysign`. Would it make sense to try and unify those in some way? Maybe create a `__copysign` function that is `constexpr` whenever it can be, and then call that from `copysign`. Here, we'd call `__copysign` directly, which has the right constexpr-ness. WDYT? I'm trying to avoid proliferating helper functions that might be useful elsewhere in just this header, since it means we'll reinvent the wheel in the future eventually.

Also, I don't understand why we're guarding the use of the builtin on `_LIBCPP_STD_VER` and not `__has_builtin`.

Same comment applies to other general functions below like `fabs` & friends. I think it may be a good idea to tackle those as a separate patch, then this patch would be a lot simpler.


================
Comment at: libcxx/include/complex:622
+#if _LIBCPP_STD_VER > 17
+    if (__libcpp_is_constant_evaluated()) {
+        bool __z_zero = __a == _Tp(0) && __b == _Tp(0);
----------------
I think we can remove the `#if _LIBCPP_STD_VER > 17` check here. In C++ <= 17, `__libcpp_is_constant_evaluated()` will just return `false`, so this `if` will never be entered (but it still needs to parse).

Also, I believe a comment saying something like "catch all corner cases that would otherwise cause invalid operations if evaluated in a constant expression" wouldn't hurt. This applies here and everywhere else where the same pattern appears.


================
Comment at: libcxx/test/std/numerics/complex.number/cases.h:183
         return zero;
-    if (std::isinf(x.real()) || std::isinf(x.imag()))
+    if (__builtin_isinf(x.real()) || __builtin_isinf(x.imag()))
         return inf;
----------------
We can't use builtins like that in the tests unless we know all supported compilers do support that builtin. Is it the case?


================
Comment at: libcxx/test/std/numerics/complex.number/complex.ops/complex_divide_complex.clang.pass.cpp:11
+
+// ADDITIONAL_COMPILE_FLAGS: -fconstexpr-steps=3500000
+
----------------
I'm quite worried by the need to add this. It means that if we somehow went crazy in the implementation and used a huge number of steps to implement the functionality, we wouldn't catch it with the tests. I'd like to suggest something like this to remove the need to pass `-fconstexpr-steps`:

```
// Helper to avoid blowing the number of steps in constexpr
template <typename F>
TEST_CONSTEXPR void chunk(std::size_t N, F f) {
  for (std::size_t i = (N/5) * 0; i != (N/5) * 1; ++i) f(i);
  for (std::size_t i = (N/5) * 1; i != (N/5) * 2; ++i) f(i);
  for (std::size_t i = (N/5) * 2; i != (N/5) * 3; ++i) f(i);
  for (std::size_t i = (N/5) * 3; i != (N/5) * 4; ++i) f(i);
  for (std::size_t i = (N/5) * 4; i != N;         ++i) f(i);
}
```

This is just an idea, and it's untested code, but basically I think we could add a generic helper to help us chunk large ranges into several for loops instead of one giant loop. Then we'd use it something like

```
TEST_CONSTEXPR_CXX20
bool test_edges() {
  const unsigned N = sizeof(testcases) / sizeof(testcases[0]);
  int classification[N];
  for (unsigned i=0; i < N; ++i)
    classification[i] = classify(testcases[i]);

  chunk(N, [](int i) {
    for (int j = 0; j != N; ++j) {
      std::complex<double> r = testcases[i] / testcases[j];
      switch (classification[i]) {
        ...
      };
    }
  });
}
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79555