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

Mark as system includes unless building standalone #40

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Krzmbrzl
Copy link

@Krzmbrzl Krzmbrzl commented Jan 4, 2024

Marking as system headers prevents the compiler from emitting warnings from these headers.

@rigtorp
Copy link
Owner

rigtorp commented Jan 27, 2024

I think I would rather fix the warnings. What warnings did you encounter?

@Krzmbrzl
Copy link
Author

This can be done additionally but new compilers will bring new warnings and even if you manage to fix all warnings in the upstream project, most folks will use a (slightly) older release and will still be affected by new warnings.
Having third party dependencies potentially issue warnings in their header files essentially makes it impossible to use warnings as errors for own projects. That's why it's more or less?good practice to mark external depenency header files as system includes.

As for the exact warnings I was seeing, I'll have to check again 👀

@rigtorp
Copy link
Owner

rigtorp commented Jan 31, 2024

Catch2 also doesn't mark the includes as SYSTEM: https://github.com/catchorg/Catch2/blob/devel/src/CMakeLists.txt#L395

If you use ExternalProject you can patch there. If you vendor you can just modify the CMakeLists.txt.

@Krzmbrzl
Copy link
Author

Krzmbrzl commented Feb 1, 2024

I already vendor because of this, but figured that others could profit from this as well if this was incorporated in the main project. And afaict there is no downside to doing this 🤷

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