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

Guard GCC pragmas #2925

Merged
merged 1 commit into from
Aug 12, 2021
Merged

Guard GCC pragmas #2925

merged 1 commit into from
Aug 12, 2021

Conversation

nlohmann
Copy link
Owner

This PR adds preprocessor checks to only use GCC pragmas with compatible compilers.

Fixes #2924

@nlohmann nlohmann added this to the Release 3.9.2 milestone Aug 11, 2021
@nlohmann nlohmann self-assigned this Aug 11, 2021
@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling e20f3f9 on issue2924 into 57d31d1 on develop.

@MHebes
Copy link

MHebes commented Aug 12, 2021

Just curious—there are other #pragma GCC s which are guarded on the __clang__ variable instead of __GNUC__. I think clang defines the latter, but gcc might not define the former. Would it make sense to change those to __GNUC__ as well?

@nlohmann
Copy link
Owner Author

Just curious—there are other #pragma GCC s which are guarded on the __clang__ variable instead of __GNUC__. I think clang defines the latter, but gcc might not define the former. Would it make sense to change those to __GNUC__ as well?

The only other place is this:

// disable documentation warnings on clang
#if defined(__clang__)
    #pragma GCC diagnostic push
    #pragma GCC diagnostic ignored "-Wdocumentation"
#endif

This pragma is meant for Clang, because only Clang has a -Wdocumentation warning. If we would change it to __GNUC__, then GCC would be confused about this unknown flag.

@nlohmann nlohmann merged commit 910fabf into develop Aug 12, 2021
@nlohmann nlohmann deleted the issue2924 branch August 12, 2021 05:58
@t-b
Copy link
Contributor

t-b commented Aug 12, 2021

@nlohmann What is missing to avoid things like this with the CI? warning-free tests on all platforms?

@nlohmann
Copy link
Owner Author

@nlohmann What is missing to avoid things like this with the CI? warning-free tests on all platforms?

I currently set no specific warning flag for MSVC. Is there something comparable to -Werror which is still pragmatic for MSVC?

@t-b
Copy link
Contributor

t-b commented Aug 13, 2021

/WX for warnings as errors and /W4 for maximum warnings. Maybe you can scavenge something from 0b8644c.

@nlohmann
Copy link
Owner Author

I'll give it a try, see #2930 - feedback more than welcome!

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

Successfully merging this pull request may close these issues.

warning C4068: unknown pragma 'GCC' on MSVC/cl
4 participants