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
Paths
| Differential D79555
[libc++] [C++20] [P0415] Constexpr for std::complex. ClosedPublic Authored by philnik on May 7 2020, 12:17 AM.
Details
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
Diff Detail
Event TimelineThere are a very large number of changes, so older changes are hidden. Show Older Changes Comment Actions 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. This revision now requires changes to proceed.May 7 2020, 10:28 AM Comment Actions @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. Comment Actions
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.
I'm seeing this on cppreference:
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?
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).
Comment Actions Fix edge cases. Fix operator* and operator/. One remaining problem.
do not pass on clang unless additional parameter --param=compile_flags='-fconstexpr-steps=4000000' is given. 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:
Comment Actions
Comment Actions 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. Comment Actions 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 Comment Actions Can you rebase onto main so that CI runs again? You'll also need to fix a few details like the cxx2a_status page which has changed.
This revision now requires changes to proceed.Feb 3 2021, 2:18 PM Comment Actions
curdeius marked an inline comment as done. curdeius added inline comments.
curdeius retitled this revision from [libc++] [P0415] [C++20] Constexpr for std::complex. to [libc++] [C++20] [P0415] Constexpr for std::complex..Feb 24 2021, 1:49 AM • Quuxplusone added inline comments.
curdeius marked 5 inline comments as done. Comment ActionsFix rebase. Apply some review comments.
curdeius marked 4 inline comments as done. Comment Actions
Comment Actions Looks like your new rebase succeeded in preserving the "unforked" implementations of operator* and operator/. They LGTM code-wise now.
curdeius marked an inline comment as done. Comment Actions
Comment Actions Sorry for sitting on this for so long. Let's push this through now.
This revision now requires changes to proceed.Jun 24 2021, 9:13 AM curdeius added a parent revision: D106364: [libc++] Add `__libcpp_copysign` conditionally constexpr overloads..Jul 21 2021, 7:13 AM
Comment Actions Commandeering to finish this, since it's been laying around for over a year and it seems like it's almost finished.
philnik marked 7 inline comments as done. Comment Actions
Comment Actions LGTM w/ green CI and assuming the answer to my question is "yes" (i.e. you didn't slip a change to the non-constexpr code path).
This revision is now accepted and ready to land.Dec 20 2022, 4:50 PM philnik marked an inline comment as done. Comment ActionsTry to fix CI
Closed by commit rG7223bcf04c4c: [libc++] [C++20] [P0415] Constexpr for std::complex. (authored by curdeius, committed by philnik). · Explain WhyJan 8 2023, 6:47 AM This revision was automatically updated to reflect the committed changes. Comment Actions FWIW, this broke the CUDA buildbots: https://lab.llvm.org/buildbot/#/builders/1/builds/41521 Tag: @tra This revision is now accepted and ready to land.Jan 10 2023, 12:06 AM Comment Actions
That looks to me like a problem with the cuda wrappers, given that we don't support cuda and we don't have any __host__ or __device__ tags. Comment Actions Thanks for the heads up @jdoerfert. It looks like Clang-cuda is re-defining <complex> in clang/lib/Headers/cuda_wrappers/complex. I'm not sure how that works but that is likely the issue. I'd wait to hear back from the Clang-cuda folks what they think the exact issue is before considering a revert, since it really seems to me like libc++ isn't doing anything wrong. Comment Actions
I'm aware of the broken bots, just didn't get a chance to deal with them yet. It's not a libc++ problem, so I don't think we need to revert. CUDA headers need to grow additional magic to make new libc++ code work on the GPU side. It's unfortunate that we need to play catch up here, but short of introducing CUDA support to libc++ itself, there's not much else we can do. I'm a bit puzzled why things didn't work, though, as normally CUDA compilation should've treated constexpr functions as __host__ __device__, and should've allowed __constexpr_fmax() etc... It's possible that we may have triggered yet another new corner case. Comment Actions Something like https://reviews.llvm.org/D141555 should unbreak CUDA. It can probably get reduced a bit, as we may not have sufficiently functional builtins on the GPU, so it's still a WIP. I can't say that I like copy/pasting libc++ code into clang headers. I think it would be great if we could directly add necessary attributes to specific functions when libc++ headers are compiled as CUDA.
philnik marked 2 inline comments as done. Comment Actions
If you're interested in doing that you can post an RFC and tell us what would be required for that. Closing this, since it doesn't seem to be a libc++ problem.
Revision Contents
Diff 487181 libcxx/docs/FeatureTestMacroTable.rst
libcxx/docs/ReleaseNotes.rst
libcxx/docs/Status/Cxx20Papers.csv
libcxx/include/__config
libcxx/include/cmath
libcxx/include/complex
libcxx/include/math.h
libcxx/include/version
libcxx/test/std/language.support/support.limits/support.limits.general/complex.version.compile.pass.cpp
libcxx/test/std/language.support/support.limits/support.limits.general/version.version.compile.pass.cpp
libcxx/test/std/numerics/complex.number/cases.h
libcxx/test/std/numerics/complex.number/cmplx.over/conj.pass.cpplibcxx/test/std/numerics/complex.number/cmplx.over/norm.pass.cpp
libcxx/test/std/numerics/complex.number/complex.member.ops/assignment_complex.pass.cpp
libcxx/test/std/numerics/complex.number/complex.member.ops/assignment_scalar.pass.cpp
libcxx/test/std/numerics/complex.number/complex.member.ops/divide_equal_complex.pass.cpp
libcxx/test/std/numerics/complex.number/complex.member.ops/divide_equal_scalar.pass.cpp
libcxx/test/std/numerics/complex.number/complex.member.ops/minus_equal_complex.pass.cpp
libcxx/test/std/numerics/complex.number/complex.member.ops/minus_equal_scalar.pass.cpp
libcxx/test/std/numerics/complex.number/complex.member.ops/plus_equal_complex.pass.cpp
libcxx/test/std/numerics/complex.number/complex.member.ops/plus_equal_scalar.pass.cpp
libcxx/test/std/numerics/complex.number/complex.member.ops/times_equal_complex.pass.cpp
libcxx/test/std/numerics/complex.number/complex.member.ops/times_equal_scalar.pass.cpp
libcxx/test/std/numerics/complex.number/complex.members/real_imag.pass.cpp
libcxx/test/std/numerics/complex.number/complex.ops/complex_divide_complex.pass.cpp
libcxx/test/std/numerics/complex.number/complex.ops/complex_divide_scalar.pass.cpp
libcxx/test/std/numerics/complex.number/complex.ops/complex_equals_complex.pass.cpp
libcxx/test/std/numerics/complex.number/complex.ops/complex_equals_scalar.pass.cpp
libcxx/test/std/numerics/complex.number/complex.ops/complex_minus_complex.pass.cpp
libcxx/test/std/numerics/complex.number/complex.ops/complex_minus_scalar.pass.cpp
libcxx/test/std/numerics/complex.number/complex.ops/complex_not_equals_complex.pass.cpp
libcxx/test/std/numerics/complex.number/complex.ops/complex_not_equals_scalar.pass.cpp
libcxx/test/std/numerics/complex.number/complex.ops/complex_plus_complex.pass.cpp
libcxx/test/std/numerics/complex.number/complex.ops/complex_plus_scalar.pass.cpp
libcxx/test/std/numerics/complex.number/complex.ops/complex_times_complex.pass.cpp
libcxx/test/std/numerics/complex.number/complex.ops/complex_times_scalar.pass.cpp
libcxx/test/std/numerics/complex.number/complex.ops/scalar_divide_complex.pass.cpp
libcxx/test/std/numerics/complex.number/complex.ops/scalar_equals_complex.pass.cpp
libcxx/test/std/numerics/complex.number/complex.ops/scalar_minus_complex.pass.cpp
libcxx/test/std/numerics/complex.number/complex.ops/scalar_not_equals_complex.pass.cpp
libcxx/test/std/numerics/complex.number/complex.ops/scalar_plus_complex.pass.cpplibcxx/test/std/numerics/complex.number/complex.ops/scalar_times_complex.pass.cpp
libcxx/test/std/numerics/complex.number/complex.ops/unary_minus.pass.cpp
libcxx/test/std/numerics/complex.number/complex.ops/unary_plus.pass.cpplibcxx/utils/generate_feature_test_macro_components.py
|
Nit: this should be // !__has_constexpr_builtin(__builtin_logb)