Add constexpr to Vector{2,3,4} and Vector<N, T>

[sthalik]

Let's sprinkle constexpr with reckless abandon.

I'm still writing tests for all vector operators and subclasses. Would like your comments as to whether this is a reasonable approach.

[mosra]

Thank you for starting the work on this. I think having post-C++11 constexpr is desirable in general, but -- as we discussed on Gitter -- I still don't know how to resolve the major blocker here:

Let's say this would get merged, enabling constexpr for everything that can have it under C++14, including for example a 4x4 matrix multiplication, or a vector cross product, and people gradually start relying on these being constexpr. But then I'd want to add SIMD-optimized variants of those two (which I already have in a private branch), and I wouldn't be able to because in order to use SIMD intrinsics I'd have to drop the constexpr again, breaking everyone's code. Which I really don't want to. So how should we solve this? I see these options:

1. Enable post-C++11 constexpr only for various mutable getters, and not for anything that is potentially SIMD-optimizable. Which probably goes against your use case, and it's also unclear where we should draw the line.
2. Enable post-C++11 constexpr only on compilers that implement function "overloading" based on whether the function is constant-evaluated (i.e., that __builtin_constant_evaluated() you mentioned). Then, when I'd want to equip those with SIMD, I'd just add an "overload". What I'm not sure about is how those "overloads" are meant to be done, everything I found was about adding some a branch inside the implementation and deciding there. Is that the way? Or can I have some hypotetical int foo() and constexpr int foo() overloads?
3. Or decide that SIMD will never be added directly to vector classes, and only used in other algorithms, and enable post-C++11 constexpr everywhere, as you do now. Sure, using SIMD in singular matrix/vector APIs is considered a "naive SIMD implementation" due to its relative inefficiency, nevertheless I'd like to keep that possibility.
4. Other options / ideas?

So far I think option 2 would be the best. Let's say with a CORRADE_CONSTEXPR14_IF_CONSTEVAL macro that's non-empty only if the compiler supports C++14 constexpr and __builtin_constant_evaluated() or equivalent is available in the compiler in the currently chosen standard.

[mosra]

Some more initial ideas for this.

Oh, and apologies in advance -- until end of October I have very little free time left, so it's very likely I won't be able to do another review round. Sorry.

[mosra]

For the tests, I think a more feasible option would be to reuse the existing test files, compile them a second time with C++14 enabled, and add a constexpr variant of each test case where C++14 constexpr got enabled.

Let's say, for example, the VectorTest::addSubtract() would be extended like this:

void VectorTest::addSubtract() {
    Vector4 a(1.0f, -3.0f, 5.0f, -10.0f);
    Vector4 b(7.5f, 33.0f, -15.0f, 0.0f);
    Vector4 c(8.5f, 30.0f, -10.0f, -10.0f);

    CORRADE_COMPARE(a + b, c);
    CORRADE_COMPARE(c - b, a);
    
    CORRADE_CONSTEXPR14 Vector4 ca(1.0f, -3.0f, 5.0f, -10.0f);
    CORRADE_CONSTEXPR14 Vector4 cb(7.5f, 33.0f, -15.0f, 0.0f);
    CORRADE_CONSTEXPR14 Vector4 cc(8.5f, 30.0f, -10.0f, -10.0f);

    CORRADE_CONSTEXPR14 Vector4 caPlusCb = ca + cb;
    CORRADE_CONSTEXPR14 Vector4 ccMinusCb = cc - cb;
    CORRADE_COMPARE(caPlusCb, cc);
    CORRADE_COMPARE(ccMinusCb, ca);
}

That way both the runtime and the constexpr tests will be localized close to each other, making it relatively easy to ensure that all constexpr variants are indeed tested. And when the runtime and compile-time implementation diverges due to SIMD, having the two cases together makes it easy to verify both still do the same in various potential corner cases.

[sthalik]

If you do that then the entire test including its runtime logic will fail to compile in case of bugs specifically pertaining to constexpr rather than fail particular subtests in CORRADE_COMPARE.

What I recommend instead is #including the .cpp file from another .cpp test file and adding a 'Constexpr' suffix via a #define. Before that, do #define CONSTEXPR constexpr and have the original test do:

#ifndef CONSTEXPR
#define CONSTEXPR
#endif

and then plaster it in front of each and every vector declaration.

If you somehow dislike #including .cpp files, it could be added a second time in CMake with -DTEST_CONSTEXPR added as as a target compile definition. Then a header file in Math/Test can contain the boilerplate pertaining to it.

[mosra]

For various reasons I'm keeping compatibility with GCC 4.8, which doesn't support C++11 yet, so the C++14-only tests need to be compiled only if the compiler supports that. Here is how it's done inside Corrade, tho the condition should probably be put into some global place (a variable in UseCorrade.cmake?) and reused, instead of copypasting it thousand times. Also, depending on how the consteval/SIMD issue gets solved, the condition may need to be different to include just compilers that have the "is constant evaluated" builtin.

Continuing with my suggestion of compiling the same test again, this would look like

if(...) 
    corrade_add_test(MathVector2Cpp14Test Vector2Test.cpp LIBRARIES MagnumMathTestLib)
    ...
    set_target_properties(
        MathVector2Cpp14Test
        ...
        PROPERTIES CORRADE_CXX_STANDARD 14)
endif()

[sthalik]

Enable post-C++11 constexpr only on compilers that implement function "overloading" based on whether the function is constant-evaluated (i.e., that __builtin_constant_evaluated() you mentioned). Then, when I'd want to equip those with SIMD, I'd just add an "overload". What I'm not sure about is how those "overloads" are meant to be done, everything I found was about adding some a branch inside the implementation and deciding there. Is that the way?

A hypothetical CONSTEVAL macro could be defined as:

#if CORRADE_CXX_STANDARD >= 202002L
#    define CONSTEVAL (std::is_constant_evaluated())
#elif defined(__GNUG__) || defined(_MSC_VER) && _MSVC_LANG >= 202002L
#    define CONSTEVAL (__builtin_is_constant_evaluated())
#elif CORRADE_CXX_STANDARD  >= 201703L
#    define CONSTEVAL constexpr (Implementation::no_simd_v)
#else
#    define CONSTEVAL (false) /* or (true) */
#endif

Other options / ideas?

There are lots of out-of-class inline functions and conversion helpers. Which is why I attempted to put constexpr on everything in the first place.

Rewriting the vector code to be a single type and having less out-of-class inline functions lying around could be an option. Regarding not wanting it to become another Eigen, that shouldn't be the case unless it thinks it's a vector (including 3D cross product) and a matrix and a column vector (including 3D cross product) at the same time. I've had my own experience with cv::MatExpr as well so I'd rather not have it become like that as well.

I'll have to keep pushing to this branch in order to run it on your CI before it even compiles in the base non-constexpr case. Before that it's hard for me to answer as to what should be done about tests or which specific functions can be enabled at which compiler and standard conformance switch.

Or can I have some hypotetical int foo() and constexpr int foo() overloads?

Unfortunately not, and C++23 didn't fix it either.

[codecov]

Codecov Report

Base: 81.26% // Head: 81.26% // Increases project coverage by +0.00% 🎉

Coverage data is based on head (beef203) compared to base (852fd16).
Patch coverage: 100.00% of modified lines in pull request are covered.

❗ Current head beef203 differs from pull request most recent head b7c5a6f. Consider uploading reports for the commit b7c5a6f to get more accurate results

Additional details and impacted files

@@           Coverage Diff           @@
##           master     #597   +/-   ##
=======================================
  Coverage   81.26%   81.26%           
=======================================
  Files         532      532           
  Lines       38965    38970    +5     
=======================================
+ Hits        31665    31670    +5     
  Misses       7300     7300           

Impacted Files	Coverage Δ
src/Magnum/Math/TypeTraits.h	100.00% <100.00%> (ø)
src/Magnum/Math/Vector.h	100.00% <100.00%> (ø)
src/Magnum/Math/Vector2.h	100.00% <100.00%> (ø)
src/Magnum/Math/Vector3.h	100.00% <100.00%> (ø)
src/Magnum/Math/Vector4.h	97.50% <100.00%> (+0.20%)	⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

[mosra]

Sorry for abruptly cancelling the CIs -- can you merge (or rebase on) current master, in particular b9726a9? I'm trying to avoid unnecessary CircleCI builds to not run out of credits at the end of the month again :]

By all means, feel free to continue to abuse the CI for testing if stuff compiles, that's what it is for anyway, just merge that commit first. Thank you. It'll now run the essential Linux jobs first and only if they pass it'll continue further with other jobs and other platforms (such as shown here for current master build).

[sthalik]

It's pretty much ready in the current state and rebased onto master. But with some exceptions:

• needs to be squashed into a single commit with a new message written.
• MAGNUM_CONSTSEXPR14 should be replaced with CORRADE_CONSTEXPR14 once that definition doesn't trip up GCC 4.8.