Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

monadic operations: fix disable #61

Merged
merged 1 commit into from
Sep 30, 2023

Conversation

szaszm
Copy link
Contributor

@szaszm szaszm commented Sep 30, 2023

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
Copy link
Owner

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.

and conditionally compile `invoke` tests, only test if `invoke` itself
is compiled
@szaszm szaszm force-pushed the monadic_fix_disable branch from 92654a7 to b9f8397 Compare September 30, 2023 15:36
@szaszm
Copy link
Contributor Author

szaszm commented Sep 30, 2023

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

@martinmoene martinmoene merged commit 45a54fa into martinmoene:master Sep 30, 2023
7 checks passed
@martinmoene
Copy link
Owner

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
Copy link
Contributor Author

szaszm commented Sep 30, 2023

Interesting viewpoint, thanks for the article recommendation.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants