Page MenuHomePhabricator

[libc++] [P0415] [C++20] Constexpr for std::complex.
Needs ReviewPublic

Authored by curdeius on May 7 2020, 12:17 AM.

Details

Reviewers
ldionne
EricWF
mclow.lists
Group Reviewers
Restricted Project
Summary

This patch adds constexpr to <complex> header: operators, member operators, and member functions (real, imag, norm, conj).

https://eel.is/c++draft/complex.numbers
https://wg21.link/p0415

TODO:

  • Test default implementation (and not only specializations for float, double, long double).

Diff Detail

Event Timeline

curdeius created this revision.May 7 2020, 12:17 AM
Herald added a project: Restricted Project. · View Herald TranscriptMay 7 2020, 12:17 AM
Herald added a reviewer: Restricted Project. · View Herald Transcript

Nice! Are you looking for a review now, or it's just a WIP patch? Ideally you'd fix the TODOs you already noted before we take a look, unless you have specific questions/concerns.

ldionne requested changes to this revision.May 7 2020, 10:28 AM

(Requesting changes so it shows up properly in my review queue)

This revision now requires changes to proceed.May 7 2020, 10:28 AM

@ldionne, I'd appreciate if you (or somebody else) could tell me what you think about the way I test the constexpr. I wanted to avoid duplication but on a second thought, I think that run-time tests can actually go through compile-time (constexpr) branch and so leave the non-constexpr branch untested.
Another thing in which I'd need guidance is how to test the primary implementation (not specializations for float, double, long double). Is there another floating-point type I can use in tests? Maybe some sort of short float?
Oh, and should integral types be tested with complex<T>?

@ldionne, I'd appreciate if you (or somebody else) could tell me what you think about the way I test the constexpr. I wanted to avoid duplication but on a second thought, I think that run-time tests can actually go through compile-time (constexpr) branch and so leave the non-constexpr branch untested.

I think it's fine -- avoiding duplication is pretty important IMO. I don't think the compiler will try to evaluate this using the constexpr evaluator if it's not marked as constexpr. If we wanted to be really really sure, we could also make sure the values we use in the test are thought to be potentially-runtime by calling a function defined in a different TU. That would be somewhat more complicated, but imagine the following setup:

// in support.cpp
template <typename T>
T identity(T t) { return t; }

template int identity(int);
template float identity(float);
// etc...

Then, let's say you have a test that is potentially constexpr like this:

template <class T>
TEST_CONSTEXPR_CXX20 bool test(T zero) {
    T lhs(1.5 + zero);
    std::complex<T> rhs(3.5 + zero, 4.5);
    std::complex<T>   x(5.0 + zero, 4.5);
    test(lhs, rhs, x);
}

You could now call it once at runtime and once at compile-time:

int main(int argc, char** argv) {
    test<float>(identity<float>(0)); // runtime test
    static_assert(test<float>(0)); // compile-time test
}

Since identity is defined in a different TU, the compiler would really have to generate code for the runtime tests. There's a few different ways to do this, another way I can think of is to piggy back on argc or just a (volatile?) global variable. But the idea is the same. If there's interest, we can look into setting this up more widely for amphibious (constexpr/not constexpr) tests.

Another thing in which I'd need guidance is how to test the primary implementation (not specializations for float, double, long double). Is there another floating-point type I can use in tests? Maybe some sort of short float?

I'm seeing this on cppreference:

The specializations std::complex<float>, std::complex<double>, and std::complex<long double> are LiteralTypes for representing and manipulating complex numbers. The effect of instantiating the template complex for any other type is unspecified.

I don't think we test other specializations in our other complex tests, do we? If not, I'd say it's OK to not test it... Maybe I'm not being imaginative enough?

Oh, and should integral types be tested with complex<T>?

Like I said above, I don't think so. Please LMK if you disagree.

What *does* need to be tested however is the interaction between std::complex<T> and U where T in {float, double, long double} and U is an integral type. But you seem to be testing this correct (e.g. in conj.pass.cpp).

libcxx/test/std/numerics/complex.number/complex.ops/scalar_plus_complex.pass.cpp
34

Nit: this spacing seems to have changed?

curdeius planned changes to this revision.May 7 2020, 3:49 PM
curdeius updated this revision to Diff 303663.Nov 7 2020, 12:53 PM

Fix edge cases. Fix operator* and operator/.
Use "shortcuts" to avoid NaN in constexpr evaluation (disallowed).

One remaining problem.
2 tests:

  • complex_divides_complex.pass.cpp
  • complex_times_complex.pass.cpp

do not pass on clang unless additional parameter --param=compile_flags='-fconstexpr-steps=4000000' is given.
That's because they test all the edge cases and each pair thereof and clang has too low a limit for the maximum number of constexpr execution steps.
I thought about splitting these tests somehow but given their (combinatoric) nature, it's pretty awkward.

I don't know how I should pass this compile flag only for these tests and only for clang (I know of ADDITIONAL_COMPILE_FLAGS but I cannot exclude gcc with that).

TODO:

  • Test (at least locally) a different floating-point type (e.g. float128) to catch missing constexpr markers in the default implementation (non-specialization).
curdeius updated this revision to Diff 303664.Nov 7 2020, 12:55 PM

Fix diff.

curdeius updated this revision to Diff 303665.Nov 7 2020, 12:59 PM
  • Revert unrelated changes in cases.h.
curdeius updated this revision to Diff 303669.Nov 7 2020, 1:29 PM
  • Add missing constexpr on generic implementation.
  • Add missing static asserts in tests.
curdeius updated this revision to Diff 303670.Nov 7 2020, 1:31 PM
  • Bump version in status.
curdeius updated this revision to Diff 303673.Nov 7 2020, 1:53 PM
  • Fix pre-C++20 modes. Ugly duplication :/.
curdeius updated this revision to Diff 303674.Nov 7 2020, 1:57 PM
  • Clean up.

I've tested the non-specialization with __float128 and fixed missing constexpr there. The tests mainly pass apart from those testing operators * and / that use copysign which is ambiguous (already in non-constexpr context so I didn't pursue it any further). It's implementation-specific anyway.

wmaxey added a subscriber: wmaxey.Nov 12 2020, 8:33 PM
ldionne requested changes to this revision.Nov 17 2020, 2:26 PM

Can you please ping once you think this is ready to review? For now, I'll "request changes" so it shows up correctly in the review queue.

This revision now requires changes to proceed.Nov 17 2020, 2:26 PM
curdeius retitled this revision from [libc++] [P0415] [C++20] - Constexpr for std::complex. to [libc++] [P0415] [C++20] Constexpr for std::complex..Nov 18 2020, 1:06 AM
curdeius edited the summary of this revision. (Show Details)
curdeius planned changes to this revision.Nov 18 2020, 1:21 AM
curdeius updated this revision to Diff 307159.Nov 23 2020, 12:08 PM
  • Hack to add a compile flag only to clang.
  • Rebase
curdeius updated this revision to Diff 307291.Nov 24 2020, 2:21 AM
  • Fix apple-clang.
  • Add header guards.

@ldionne, it's ready for review (at least the first glance).