Add macros for conditional constexpr by sthalik · Pull Request #152 · mosra/corrade

sthalik · 2022-10-09T06:42:03Z

This is going to be used in Magnum vectors.

codecov · 2022-10-09T07:56:11Z

Codecov Report

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

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

❗ 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

@@            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	Coverage Δ
src/Corrade/Containers/BitArray.h	`92.30% <0.00%> (-7.70%)`	⬇️
src/Corrade/Containers/BitArray.cpp	`94.28% <0.00%> (-5.72%)`	⬇️
src/Corrade/Utility/Path.cpp	`85.49% <0.00%> (-2.82%)`	⬇️
src/Corrade/Utility/Resource.cpp	`95.65% <0.00%> (-1.23%)`	⬇️
src/Corrade/PluginManager/AbstractManager.cpp	`97.27% <0.00%> (-1.14%)`	⬇️
src/Corrade/Utility/Format.cpp	`98.37% <0.00%> (-0.78%)`	⬇️
src/Corrade/Utility/JsonWriter.cpp	`99.30% <0.00%> (-0.70%)`	⬇️
src/Corrade/Containers/String.cpp	`95.25% <0.00%> (-0.62%)`	⬇️
src/Corrade/PluginManager/AbstractPlugin.cpp	`80.32% <0.00%> (-0.03%)`	⬇️
src/Corrade/Cpu.h	`100.00% <0.00%> (ø)`
... and 60 more

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 · 2022-10-09T16:36:58Z

+#define CORRADE_CXX14_CONSTEXPR constexpr
+#else
+#define CORRADE_CXX14_CONSTEXPR
+#endif


There's already CORRADE_CONSTEXPR14 in Corrade/Utility/Macros.h, and I'd like the other two to follow a similar naming and be in the same header.

But also please see my comment in mosra/magnum#597 about having a special macro for C++14+ constexpr only if a "is constant evaluated" builtin is available.

Alright. Seems like my original proposal wasn't sufficient for what we're trying to achieve. It's not too useful for any of CORRADE_CONSTEXPR{17,20,23} to be defined -- C++14 relaxed constexpr is already enough for anything Vector has to handle. But CORRADE_CONSTEXPR14 needs to reflect whether it works in practice (__cpp_constexpr is accurate here). Then we have several kinds of constexpr functions:

C++11 semantics -- constexpr

constexpr with C++14 rules, no need for inline asm or intrinsics -- CORRADE_CONSTEXPR14

ones that need SIMD -- CORRADE_CONSTEVAL14, #ifndef CORRADE_CONSTEVAL

The last case can be used like that:

template<typename T> CORRADE_CONSTEVAL14 inline T foo(T x) { #ifdef CORRADE_CONSTEVAL if CORRADE_CONSTEVAL { // do something traditional to 'x' } else #endif { // do something funny to 'x' } }

If you want to enable constexpr when lacking CORRADE_CONSTEVAL but when SIMD has been disabled, then it could be set to constexpr(true) as long as __cpp_if_constexpr >= 201606 holds (aka C++17).

I think it's better to remove CORRADE_CONSTEVAL14 from this pull request and put it in Magnum with a name such as MAGNUM_VECTOR_CONSTEVAL14. It's too specialized for a global definition. Here it is for reference:

#ifdef CORRADE_CONSTEVAL #define CORRADE_CONSTEVAL14 CORRADE_CONSTEXPR14 #else #define CORRADE_CONSTEVAL14 #endif

mosra · 2022-10-29T17:59:52Z

 */
-#if CORRADE_CXX_STANDARD >= 201402
+#if defined __cpp_constexpr && __cpp_constexpr >= 201304 || \
+    defined CORRADE_TARGET_MSVC && _MSC_VER >= 1910 && CORRADE_CXX_STANDARD >= 201402L


Shouldn't this contain a bunch of parentheses, to ensure && and || are evaluated in the correct order? Same below.

Also, TIL that defined can be without parentheses. Nevertheless, could you add them there, just for consistency with all other occurences of defined()? Thanks.

What's the L needed for? I don't think it's essential. Or do you get some compiler warnings without?

Shouldn't this contain a bunch of parentheses, to ensure && and || are evaluated in the correct order?

It always binds tightest in the order: >=, &&, ||. I'm going to add the parentheses anyway.

What's the L needed for? I don't think it's essential.

Good to know.

I'm going to add the parentheses anyway.

Thank you :) I can never remember the order, and GCC sometimes warns about "|| within &&" and such if I forget the braces, so I suspect it's always different that I would expect :D

This table always helps: https://en.cppreference.com/w/cpp/language/operator_precedence.

Wait so the second MSVC-specific part is no longer needed? I don't see it in the last revision.

Good catch! It was only in the Magnum PR.

This is still missing the parentheses...

mosra · 2022-10-29T18:11:37Z

@@ -394,12 +394,19 @@ Expands to @cpp constexpr @ce on C++14 and newer, empty on C++11. Useful for
 selectively marking functions that make use of C++14 relaxed constexpr rules.


Since the macro is quite a bit more complex now, I'm thinking a test could be a good idea -- there's src/Corrade/Utility/MacrosCppXYTest.cpp which currently test C++17 and 20 features, so a new MacrosCpp14Test.cpp (built only on C++14-capable compilers) would contain a test for this macro:

... CORRADE_CONSTEXPR14 int sumInAStupidWay(int number) { int sum = 0; for(int i = 0; i != number; ++i) sum += i; return sum; } void MacrosCpp14Test::constexpr14() { #ifndef CORRADE_CONSTEXPR14 // errr, test if it's actually non-empty, in some way CORRADE_SKIP("CORRADE_CONSTEXPR14 not available on this compiler."); #else constexpr int sum = sumInAStupidWay(17); CORRADE_COMPARE(sum, 153); #endif } }}}} CORRADE_TEST_MAIN(Corrade::Utility::Test::MacrosCpp14Test)

Because without the test it's hard to verify that the macro is actually correctly defined only where C++14 constexpr works.

And the docs should be extended to mention the compiler versions on which the macro is defined.

Oh, and similar test for the other macro, there's already a MacrosCpp20Test.cpp so just add a new case there.

Generally when testing constexpr functions I recommend putting the compiler in a situation where it can't weasel out of performing the computation at compile-time. Hence the static assert.

Ah yes, good idea. Except that now you're using the C++17 variant of it ;)

I think I managed to test whether the function is constexpr through SFINAE.

Still needs the documentation update...

mosra

👀

mosra · 2022-10-29T20:08:59Z

+MacrosCpp14Test::MacrosCpp14Test() {
+    addTests({&MacrosCpp14Test::constexpr14});
+}


Can you follow the same method order as in other tests? Constructor first, helper functions defined next to where they get used, etc.

mosra · 2022-10-29T20:10:44Z

+template<int> struct sfinae : std::true_type{};
+template<class T> sfinae<(T::sumInAStupidWay(0), 0)> check(int);
+template<class> std::false_type check(...);
+template<class T> struct hasRelaxedConstexpr : decltype(check<T>(0)){};


But this way you're not testing if the macro is actually defined to constexpr when it should be, no? Like, the test would pass even if CORRADE_CONSTEXPR14 would be empty always, or if you wouldn't use it at all.

... and you're not using the static assert anymore :P

There really needs to be something based on whether the macro is empty or not.

Alright, so I added an #if 0 so that it can be verified. Sadly static assert broke the build.

I don't like this solution, to be honest. Because it requires manual fiddling with the #if 0, it's impossible to test on the CI, and thus it's the same as if the whole test file wouldn't be here.

What then, you want the test to fail rather than skip?

The test would pass even if CORRADE_CONSTEXPR14 wasn't used anywhere in it, that's the problem I have with it. It's not testing the thing it should be testing, so it's useless.

It'll skip on an old compiler, that's why that sfinae logic is there in the first place. Toggle the #if 0 and see.

mosra · 2022-10-29T22:37:43Z

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 · 2022-10-29T22:58:36Z

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 · 2022-10-30T11:36:11Z

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 fbb9305, with the corresponding test in bee514f, for both the C++11 and the C++14 case. All CIs pass.

Co-authored-by: Stanislaw Halik <sthalik@misaki.pl>

mosra · 2022-10-30T11:42:54Z

+#if (defined(__clang__) && __clang_major__ >= 9) || (defined(__GNUG__) && __GNUG__ >= 9) || (defined(_MSC_VER) && _MSC_VER >= 1931)
+#define CORRADE_CONSTEVAL (__builtin_is_constant_evaluated())
+#elif CORRADE_CXX_STANDARD >= 202002
+#define CORRADE_CONSTEVAL (std::is_constant_evaluated())


Since this depends on <type_traits>, I'm thinking the macro should go to TypeTraits.h instead. The Macros.h header deliberately doesn't pull in any STL headers.

mosra · 2022-10-30T11:43:50Z

+extension. In which case it may be used under C++14 relaxed constexpr rules.
+*/
+#if (defined(__clang__) && __clang_major__ >= 9) || (defined(__GNUG__) && __GNUG__ >= 9) || (defined(_MSC_VER) && _MSC_VER >= 1931)
+#define CORRADE_CONSTEVAL (__builtin_is_constant_evaluated())


I'm thinking CORRADE_IS_CONSTANT_EVALUATED would be a less confusing name.

mosra · 2022-10-30T11:45:15Z

+This support is available on all C++20 capable compilers, but also certain
+ones (Clang 9, GCC 9, MSVC 17.1) that expose the feature as a non-portable
+extension. In which case it may be used under C++14 relaxed constexpr rules.
+*/


Thanks for the docs, now it's just a test missing :) TypeTraitsCpp20Test.cpp

sthalik · 2022-10-30T13:06:22Z

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

This is useful for future SIMD support in Magnum vectors. v2: Add test.

mosra reviewed Oct 9, 2022

View reviewed changes

mosra added this to the 2022.0a milestone Oct 9, 2022

sthalik force-pushed the pr/constexpr-macros branch 8 times, most recently from a727881 to 7220cb9 Compare October 10, 2022 12:29

sthalik mentioned this pull request Oct 19, 2022

Math: workaround constexpr conversion operator on MSVC mosra/magnum#602

Closed

sthalik force-pushed the pr/constexpr-macros branch 2 times, most recently from 38044a3 to 5723da7 Compare October 22, 2022 21:23

mosra reviewed Oct 29, 2022

View reviewed changes

sthalik force-pushed the pr/constexpr-macros branch 6 times, most recently from 9f1acfa to ba036e1 Compare October 29, 2022 20:07

mosra reviewed Oct 29, 2022

View reviewed changes

sthalik force-pushed the pr/constexpr-macros branch 4 times, most recently from 302d124 to 6d68526 Compare October 29, 2022 21:13

mosra closed this Oct 29, 2022

mosra reopened this Oct 29, 2022

mosra referenced this pull request Oct 30, 2022

Utility: fix the CORRADE_CONSTEXPR14 macro to be empty on MSVC2015.

fbb9305

Co-authored-by: Stanislaw Halik <sthalik@misaki.pl>

mosra reviewed Oct 30, 2022

View reviewed changes

sthalik force-pushed the pr/constexpr-macros branch 3 times, most recently from c95cbd4 to e6c22d0 Compare October 30, 2022 13:04

Utility: add a macro for conditional compilation

a0a1601

This is useful for future SIMD support in Magnum vectors. v2: Add test.

sthalik force-pushed the pr/constexpr-macros branch from e6c22d0 to a0a1601 Compare October 30, 2022 13:10

mosra mentioned this pull request Dec 29, 2022

constexpr StringView without literals #123

Open

sthalik closed this Sep 12, 2023

sthalik deleted the pr/constexpr-macros branch September 12, 2023 02:52

		@@ -394,12 +394,19 @@ Expands to @cpp constexpr @ce on C++14 and newer, empty on C++11. Useful for
		selectively marking functions that make use of C++14 relaxed constexpr rules.

Conversation

sthalik commented Oct 9, 2022

Uh oh!

codecov Bot commented Oct 9, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mosra Oct 29, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mosra left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sthalik Oct 29, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mosra commented Oct 29, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sthalik commented Oct 29, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mosra commented Oct 30, 2022

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sthalik commented Oct 30, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

codecov Bot commented Oct 9, 2022 •

edited

Loading

mosra Oct 29, 2022 •

edited

Loading

sthalik Oct 29, 2022 •

edited

Loading

mosra commented Oct 29, 2022 •

edited

Loading

sthalik commented Oct 29, 2022 •

edited

Loading

sthalik commented Oct 30, 2022 •

edited

Loading