search

mosra / corrade

C++11 multiplatform utility library
https://magnum.graphics/corrade/
Other
486 stars 107 forks source link

Add macros for conditional constexpr #152

Closed sthalik closed 1 year ago

sthalik commented 2 years ago

This is going to be used in Magnum vectors.

codecov[bot] commented 2 years ago

Codecov Report

Base: 97.39% // Head: 97.94% // Increases project coverage by +0.55% :tada:

Coverage data is based on head (8f81721) compared to base (4ff543d). Patch has no changes to coverable lines.

:exclamation: Current head 8f81721 differs from pull request most recent head ece28cb. Consider uploading reports for the commit ece28cb to get more accurate results

Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #152 +/- ## ========================================== + Coverage 97.39% 97.94% +0.55% ========================================== Files 131 133 +2 Lines 10248 10865 +617 ========================================== + Hits 9981 10642 +661 + Misses 267 223 -44 ``` | [Impacted Files](https://codecov.io/gh/mosra/corrade/pull/152?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Vladim%C3%ADr+Vondru%C5%A1) | Coverage Δ | | |---|---|---| | [src/Corrade/Containers/BitArray.h](https://codecov.io/gh/mosra/corrade/pull/152/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Vladim%C3%ADr+Vondru%C5%A1#diff-c3JjL0NvcnJhZGUvQ29udGFpbmVycy9CaXRBcnJheS5o) | `92.30% <0.00%> (-7.70%)` | :arrow_down: | | [src/Corrade/Containers/BitArray.cpp](https://codecov.io/gh/mosra/corrade/pull/152/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Vladim%C3%ADr+Vondru%C5%A1#diff-c3JjL0NvcnJhZGUvQ29udGFpbmVycy9CaXRBcnJheS5jcHA=) | `94.28% <0.00%> (-5.72%)` | :arrow_down: | | [src/Corrade/Utility/Path.cpp](https://codecov.io/gh/mosra/corrade/pull/152/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Vladim%C3%ADr+Vondru%C5%A1#diff-c3JjL0NvcnJhZGUvVXRpbGl0eS9QYXRoLmNwcA==) | `85.49% <0.00%> (-2.82%)` | :arrow_down: | | [src/Corrade/Utility/Resource.cpp](https://codecov.io/gh/mosra/corrade/pull/152/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Vladim%C3%ADr+Vondru%C5%A1#diff-c3JjL0NvcnJhZGUvVXRpbGl0eS9SZXNvdXJjZS5jcHA=) | `95.65% <0.00%> (-1.23%)` | :arrow_down: | | [src/Corrade/PluginManager/AbstractManager.cpp](https://codecov.io/gh/mosra/corrade/pull/152/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Vladim%C3%ADr+Vondru%C5%A1#diff-c3JjL0NvcnJhZGUvUGx1Z2luTWFuYWdlci9BYnN0cmFjdE1hbmFnZXIuY3Bw) | `97.27% <0.00%> (-1.14%)` | :arrow_down: | | [src/Corrade/Utility/Format.cpp](https://codecov.io/gh/mosra/corrade/pull/152/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Vladim%C3%ADr+Vondru%C5%A1#diff-c3JjL0NvcnJhZGUvVXRpbGl0eS9Gb3JtYXQuY3Bw) | `98.37% <0.00%> (-0.78%)` | :arrow_down: | | [src/Corrade/Utility/JsonWriter.cpp](https://codecov.io/gh/mosra/corrade/pull/152/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Vladim%C3%ADr+Vondru%C5%A1#diff-c3JjL0NvcnJhZGUvVXRpbGl0eS9Kc29uV3JpdGVyLmNwcA==) | `99.30% <0.00%> (-0.70%)` | :arrow_down: | | [src/Corrade/Containers/String.cpp](https://codecov.io/gh/mosra/corrade/pull/152/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Vladim%C3%ADr+Vondru%C5%A1#diff-c3JjL0NvcnJhZGUvQ29udGFpbmVycy9TdHJpbmcuY3Bw) | `95.25% <0.00%> (-0.62%)` | :arrow_down: | | [src/Corrade/PluginManager/AbstractPlugin.cpp](https://codecov.io/gh/mosra/corrade/pull/152/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Vladim%C3%ADr+Vondru%C5%A1#diff-c3JjL0NvcnJhZGUvUGx1Z2luTWFuYWdlci9BYnN0cmFjdFBsdWdpbi5jcHA=) | `80.32% <0.00%> (-0.03%)` | :arrow_down: | | [src/Corrade/Cpu.h](https://codecov.io/gh/mosra/corrade/pull/152/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Vladim%C3%ADr+Vondru%C5%A1#diff-c3JjL0NvcnJhZGUvQ3B1Lmg=) | `100.00% <0.00%> (ø)` | | | ... and [60 more](https://codecov.io/gh/mosra/corrade/pull/152/diff?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Vladim%C3%ADr+Vondru%C5%A1) | | Help us with your feedback. Take ten seconds to tell us [how you rate us](https://about.codecov.io/nps?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Vladim%C3%ADr+Vondru%C5%A1). Have a feature suggestion? [Share it here.](https://app.codecov.io/gh/feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Vladim%C3%ADr+Vondru%C5%A1)

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

mosra commented 2 years ago

Actually, I'm still not sure what is the main reason the change to CORRADE_CONSTEXPR14 is needed.

Apart from MSVC, are there any relevant compilers that report C++14 as supported via the __cplusplus macro but don't actually support C++14 constexpr (is that scenario even allowed by the standard?)? I suspect not, or at least no versions that'd still be widely used in distros or OSes. And since you have another dedicated check for MSVC >= 2017 in the #if (does that mean __cpp_constexpr didn't work?), I think the macro could still check for CORRADE_CXX_STANDARD >= 201402 like before. Which significantly simplifies everything, since the test doesn't need to handle the case of "C++14 supported but C++14 constexpr not" at all.

Then, for MSVC, isn't it just about excluding MSVC 2015? It would boil down to 

#if CORRADE_CXX_STANDARD >= 201402 && !defined(CORRADE_MSVC2015_COMPATIBILITY)
#define CORRADE_CONSTEXPR14 constexpr
...
#endif

and using CORRADE_MSVC2015_COMPATIBILITY to special-case the test, just for this one compiler, assuming sanity everywhere else:

void MacrosCpp14Test::constexpr14() {
    #ifdef CORRADE_MSVC2015_COMPATIBILITY
    CORRADE_SKIP("CORRADE_CONSTEXPR14 not available on MSVC2015.");
    #else
    constexpr int sum = sumInAStupidWay(17);
    CORRADE_COMPARE(sum, 153);
    #endif
}

[Sorry for the accidental close, pressed too many buttons at once.]

sthalik commented 2 years ago

Apart from MSVC, are there any relevant compilers that report C++14 as supported via the __cplusplus macro but don't actually support C++14 constexpr

Compilers tend to expose -std=c++14 before it's fully implemented or even before the standard is finalized (see how different versions of concepts or ranges were implemented). Quite a few of the CI bots are running GCC 4.8 which has this problem.

https://gcc.gnu.org/projects/cxx-status.html -- 4.3 -> 5.0 https://clang.llvm.org/cxx_status.html - 2.9 -> 3.4 https://learn.microsoft.com/en-us/cpp/overview/visual-cpp-language-conformance - 2013 12.0 -> 2017 15.0*

Which significantly simplifies everything, since the test doesn't need to handle the case of "C++14 supported but C++14 constexpr not" at all.

The whole point is to handle that for cases such as Magnum vector code. Or am I meant to litter it with _MSC_VER and __GNUG__?

mosra commented 2 years ago

Compilers tend to expose -std=c++14 before it's fully implemented

Yes, -std=c++xy is, but I'm talking about the __cplusplus macro, which is defined to 2014xy only on GCC 5+. Which solves the problem for GCC 4.8, it was already working there correctly even before this PR. Old Clangs are irrelevant nowadays, so there's no point in distinguishing anything there; and for MSVC we have to special-case anyway.

I'm not trying to solve the general case of "is feature X supported completely and without bugs on compiler Y", here it's just about having the simplest possible condition for "is C++14 constexpr supported on compilers we care about", and since C++14 is rather old and the relevant compilers are dying out, it can be coarser than, say, checking for consteval support. Which is what #if CORRADE_CXX_STANDARD >= 201402 && !defined(CORRADE_MSVC2015_COMPATIBILITY) does.

The whole point is to handle that for cases such as Magnum vector code. Or am I meant to litter it with _MSC_VER and __GNUG__?

What? Why? The whole point here is to define the macro in a way that does the right thing without a need for extra checks every time it's used, and have a test that verifies it indeed does the right thing.

I guess easiest is if I just do the thing instead of talking about it. The CORRADE_CONSTEXPR14 part is merged as fbb9305265fd2fe89e45893108f9e0ee686c341d, with the corresponding test in bee514f31d1d58e0905683dab3d23a903ff930a8, for both the C++11 and the C++14 case. All CIs pass.

sthalik commented 2 years ago

Note, it's specifically not meant to be used as if constexpr(std::is_constant_evaluated()) ..., because in that case it always returns true.