search

mosra / magnum

Lightweight and modular C++11 graphics middleware for games and data visualization
https://magnum.graphics/
Other
4.74k stars 439 forks source link

Math: workaround constexpr conversion operator on MSVC #602

Closed sthalik closed 1 year ago

sthalik commented 1 year ago

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 commented 1 year ago

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

mosra commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

The conversion operator

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

Works in any of these cases:

Is the static_cast solution acceptable to you?

mosra commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

Things left to do here:

sthalik commented 1 year ago

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 commented 1 year ago

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