martinmoene / expected-lite

expected lite - Expected objects in C++11 and later in a single-file header-only library
Boost Software License 1.0
375 stars 34 forks source link

monadic operations: fix disable #61

Closed szaszm closed 12 months ago

szaszm commented 12 months ago

When compiling with -DCMAKE_CXX_FLAGS=-Dnsel_P2505R=, as documented in the README to disable P2505 support, the macro expansions resulted in an empty string, leading to errors like this on all expansions:

/home/szaszm/expected-lite/test/expected.t.cpp:1243:17: error: operator '>=' has no left operand
 1243 | #if nsel_P2505R >= 4
      |                 ^~

This fixes that by changing the version checks to use (nsel_P2505R+0). Since R0 is not implemented, and this results in an expansion to (+0) in the case of an empty nsel_P2505R macro, and disabling the feature.

Also adds missing #if blocks around the invoke test. related error:

/home/szaszm/expected-lite/test/expected.t.cpp:2181:51: error: ‘invoke’ is not a member of ‘nonstd::expected_lite::detail’
 2181 |     static_assert( nonstd::expected_lite::detail::invoke( &A::x, A{21} ) == 21, "" );

Alternatively, we could assign the value -1 (or anything less than 3) to disabled in the README, which would not need the (+0) workaround. Just write a comment if you prefer that, and I can quickly update the pull request.

martinmoene commented 12 months ago

Happy to see this addressed.

Initially i noticed the use of the empty macro, and thought '0', but forgot.

I generally favour defined macros and using #if. Have to dig up the relevant article (don't hold your breath).

Documenting zero (and accepting two or lower) to disable this extension seems fine to me.

szaszm commented 12 months ago

This was an oversight on my part. Changed to documenting '0' as disabled in the README.

martinmoene commented 12 months ago

With respect to preferring #if over #ifdef, see article Advanced preprocessor tips and tricks by iar, section #if versus #ifdef which one should we use? at the bottom.

szaszm commented 12 months ago

Interesting viewpoint, thanks for the article recommendation.