Math: workaround constexpr conversion operator on MSVC

[sthalik]

Note: for the BitVector part, it may fail on old compilers unless you merge the first thunk of mosra/corrade#152 (changing CORRADE_CONSTEXPR14 to be based on __cpp_constexpr value).

[sthalik]

As for the tests, it fails even on MSVC 2022 with /pedantic-.

[mosra]

I don't understand what this PR is about. I need more context. Why are the constexprs defined away for MSVC? What does it fix? Why it needs to be fixed if it worked before?

[sthalik]

Here's the error message for the BitVector thing which is a real bug:

% msvc64 ninja MathBitVectorTest.exe
[1/2] Building CXX object magnum\src\Magnum\Math...iles\MathBitVectorTest.dir\BitVectorTest.cpp.obj
FAILED: magnum/src/Magnum/Math/Test/CMakeFiles/MathBitVectorTest.dir/BitVectorTest.cpp.obj
C:\PROGRA~1\MICROS~2\2022\COMMUN~1\VC\Tools\MSVC\1433~1.316\bin\Hostx64\x64\cl.exe  /nologo /TP -DNO
MINMAX -DUNICODE -DWIN32_LEAN_AND_MEAN -D_CRT_SECURE_NO_WARNINGS -D_HAS_EXCEPTIONS=0 -D_SCL_SECURE_N
O_WARNINGS -D_UNICODE -D_USE_MATH_DEFINES=1 -IF:\dev\game\magnum\src -IF:\dev\game\build-msvc\magnum
\src -IF:\dev\game\corrade\src -IF:\dev\game\build-msvc\corrade\src -diagnostics:caret -Zc:preproces
sor -wd4117 -Zi -Zf -Zo -bigobj -cgthreads1 -vd0 -permissive- -Zc:throwingNew -Zc:lambda -Zc:inline
 -O2 -Oit -Oy- -Ob3 -fp:fast -GS- -GF -GL -Gw -Gy  -MD /permissive- -wd4244 -wd4312 -wd4251 -wd4456
-external:W0 -external:anglebrackets /W4 /wd4127 /wd4251 /wd4244 /wd4267 /wd4324 /wd4351 /wd4373 /wd
4458 /wd4510 /wd4610 /wd4512 /wd4661 /wd4702 /wd4706 /wd4800 /wd4910 -EHsc -std:c++20 /showIncludes
/Fomagnum\src\Magnum\Math\Test\CMakeFiles\MathBitVectorTest.dir\BitVectorTest.cpp.obj /Fdmagnum\src\
Magnum\Math\Test\CMakeFiles\MathBitVectorTest.dir\ /FS -c F:\dev\game\magnum\src\Magnum\Math\Test\Bi
tVectorTest.cpp
F:\dev\game\magnum\src\Magnum\Math\Test\BitVectorTest.cpp(198,22): error C2131: expression did not e
valuate to a constant
    constexpr BVec3 d(b);
                     ^
F:\dev\game\magnum\src\Magnum\Math\Test\BitVectorTest.cpp(198,23): note: failure was caused by call
of undefined function or one not declared 'constexpr'
    constexpr BVec3 d(b);
                      ^
F:\dev\game\magnum\src\Magnum\Math\Test\BitVectorTest.cpp(198,23): note: see usage of 'Magnum::Math:
:BitVector<3>::operator bool'
    constexpr BVec3 d(b);
                      ^
ninja: build stopped: subcommand failed.
<1> dev/game/build-msvc %                             

And here's the MSVC workaround:

% msvc64 ninja MathComplexTest
[1/2] Building CXX object magnum\src\Magnum\Math...akeFiles\MathComplexTest.dir\ComplexTest.cpp.obj
FAILED: magnum/src/Magnum/Math/Test/CMakeFiles/MathComplexTest.dir/ComplexTest.cpp.obj
C:\PROGRA~1\MICROS~2\2022\COMMUN~1\VC\Tools\MSVC\1433~1.316\bin\Hostx64\x64\cl.exe  /nologo /TP -DCO
RRADE_GRACEFUL_ASSERT -DNOMINMAX -DUNICODE -DWIN32_LEAN_AND_MEAN -D_CRT_SECURE_NO_WARNINGS -D_HAS_EX
CEPTIONS=0 -D_SCL_SECURE_NO_WARNINGS -D_UNICODE -D_USE_MATH_DEFINES=1 -IF:\dev\game\magnum\src -IF:\
dev\game\build-msvc\magnum\src -IF:\dev\game\corrade\src -IF:\dev\game\build-msvc\corrade\src -diagn
ostics:caret -Zc:preprocessor -wd4117 -Zi -Zf -Zo -bigobj -cgthreads1 -vd0 -permissive- -Zc:throwing
New -Zc:lambda -Zc:inline  -O2 -Oit -Oy- -Ob3 -fp:fast -GS- -GF -GL -Gw -Gy  -MD /permissive- -wd424
4 -wd4312 -wd4251 -wd4456 -external:W0 -external:anglebrackets /W4 /wd4127 /wd4251 /wd4244 /wd4267 /
wd4324 /wd4351 /wd4373 /wd4458 /wd4510 /wd4610 /wd4512 /wd4661 /wd4702 /wd4706 /wd4800 /wd4910 -EHsc
 -std:c++20 /showIncludes /Fomagnum\src\Magnum\Math\Test\CMakeFiles\MathComplexTest.dir\ComplexTest.
cpp.obj /Fdmagnum\src\Magnum\Math\Test\CMakeFiles\MathComplexTest.dir\ /FS -c F:\dev\game\magnum\src
\Magnum\Math\Test\ComplexTest.cpp
F:\dev\game\magnum\src\Magnum\Math\Test\ComplexTest.cpp(277,23): error C2440: 'initializing': cannot
 convert from 'const Magnum::Math::Test::`anonymous-namespace'::Complex' to 'float'
    constexpr Cmpl d(b);
                      ^
F:\dev\game\magnum\src\Magnum\Math\Test\ComplexTest.cpp(277,22): note: No user-defined-conversion op
erator available that can perform this conversion, or the operator cannot be called
    constexpr Cmpl d(b);
                     ^
F:\dev\game\magnum\src\Magnum\Math\Test\ComplexTest.cpp(277,21): error C2131: expression did not eva
luate to a constant
    constexpr Cmpl d(b);
                    ^
F:\dev\game\magnum\src\Magnum\Math\Test\ComplexTest.cpp(277,21): note: a non-constant (sub-)expressi
on was encountered
    constexpr Cmpl d(b);
                    ^
ninja: build stopped: subcommand failed.

Your SFINAE to/from converter looks legit and has CE for everything it calls.

[mosra]

Well, you're putting it as if Magnum wouldn't be compiling on MSVC in general, which isn't true, because the MSVC CIs I have are passing, and were so for the last several years and several compiler versions.

So the core of the problem is somewhere else, and that's the investigation that needs to be done upfront, and only then a PR with sufficient details as well as code comments can be opened, otherwise we're both wasting time, really.

The constexpr conversion clearly works in some cases, and clearly doesn't in yours. I suppose it can be either due to a new MSVC version and a new regression in it (hopefully not?), or (more likely) due to using C++20. So the real question is -- what behavior changes with -std=c++20, and is it a compiler bug (is it known? do we have a link to track?), or something that has to be fixed in Magnum? Without knowing what and why, I'd soon end up with half of all tests wrapped in #ifndef CORRADE_TARGET_MSVC and that would help nobody.

[sthalik]

The conversion operator

constexpr Math::Bezier<2, 2, Float> b{};
constexpr QBezier2D d(b);

Works in any of these cases:

• it's called using a static cast (i.e. constexpr auto d = static_cast<QBezier2D>(b);)
• explicit is removed from operator U()
• a constructor to is added manually (i.e. constexpr QBezier2D(float x0, float x1, ...); rather than using aggregate initialization
• constexpr is removed
• MSVC 2022 compiler mode is switched to C++17

Is the static_cast solution acceptable to you?

[mosra]

That... sounds like a MSVC bug, no? Ah well. I'm trying to find a related bugreport but no success.

Does constexpr auto d = QBezier(b); work?

An acceptable solution would be to do something like this in all cases that are problematic:

/* A comment why is such a silly workaround needed and the exact _MSC_VER it failed on (such 
   as 1967) so it can be removed if a future version fixes it */
#if defined(CORRADE_TARGET_MSVC) && !defined(CORRADE_TARGET_CLANG_CL) && CORRADE_CXX_STANDARD >= 2020xy
constexpr auto d = QBezier(b);
#else
constexpr QBezier d(b);
#endif

[sthalik]

Can you change CORRADE_CONSTEXPR14 in the vein of mosra/corrade#152?

#if defined __cpp_constexpr && __cpp_constexpr >= 201304
#define CORRADE_CONSTEXPR14 constexpr
#else
#define CORRADE_CONSTEXPR14
#endif

It's going to fix GCC 4.8 build.

This could be investigated, for instance for working around broken MSVC versions:

#ifndef __cpp_constexpr
#error "foo"
#endif

It should only fail on MSVC 2015. For instance, run it on all CI instances without compiling or running tests to save time.

[mosra]

Things left to do here:

• Did you manage to find an existing MSVC bugreport for this?

• If not, would it be possible for you to make a trimmed-down repro case and report it? I fear you're going to hit this in various other places sooner or later anyway, so letting them know and giving them a chance to fix this might be a good idea.

• If not, can you check if it's a MSVC2022-specific issue? Not sure if MSVC2019 supports C++20 enough, tho, so maybe it's really just due to new features in MSVC 2022. Could you then optimistically limit it to just MSVC 2022 (adding _MSC_VER < 1940 to each)?

• I still want that explanatory comment for each, as I suggested above. Ideally with the bugreport link, if not then at least mentioning what is the root cause and which exact _MSC_VER is it failing on. Otherwise -- again, as I said above -- it'll inevitably result in a ton of #ifdefs for which nobody remembers the reason anymore.

• Can you please indent the preprocessor to align with surrounding code?

• Can you move the BitVector part to Add constexpr to Vector{2,3,4} and Vector<N, T> #597? It's the only piece that makes the CI fail and blocks this PR from being merged.

[sthalik]

Here's the MSVC bug report, you can subscribe if you want: <https://developercommunity.visualstudio.com/t/MSVC-1933-fails-to-compile-valid-code-u/10185268>. The guys at MSFT have been stellar so far, they fixed 3 problems of mine.

[mosra]

Awesome, thank you! I can take it from here -- merged as c1578c0.