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

Fix warnings in public headers #460

Open
kring opened this issue Mar 16, 2022 · 4 comments
Open

Fix warnings in public headers #460

kring opened this issue Mar 16, 2022 · 4 comments
Labels
good first issue Good for newcomers quality Improve code quality or encourage developer success

Comments

@kring
Copy link
Member

kring commented Mar 16, 2022

Somewhere along the way, we started identifying our public header files to CMake as SYSTEM. This is the right-ish thing to do for libraries that consume cesium-native via submodule / add_directory, so that a project stricter warnings won't flag a bunch of warnings in cesium-native headers. But it means that legitimate warnings in the header files, even when building cesium-native, are ignored. I noticed this because apparently some old versions of CMake don't treat SYSTEM headers specially, and the build had a whole bunch of warnings.

So we should:

  • Tweak our CMake config so that the headers are SYSTEM outside cesium-native, but non-system within it.
  • Fix the warnings
@kring kring added quality Improve code quality or encourage developer success good first issue Good for newcomers labels May 9, 2022
@lilleyse
Copy link
Contributor

This also needs to be fixed so that clang-tidy can report errors for cesium-native's header files.

@kring
Copy link
Member Author

kring commented Nov 20, 2024

@lilleyse have you already done some work on this? I've run into a couple of problems lately where cesium-native was building fine, but when we tried to use it Unreal there were new (legitimate) warnings that required us to go back and fix cesium-native. So I'd like to address this one, but don't want to duplicate work that you might have already done.

@lilleyse
Copy link
Contributor

@kring I haven't done any work on this, feel free to take this one.

@azrogers
Copy link
Contributor

I've introduced a solution for this in the clang-tidy PR #1004 using the PROJECT_IS_TOP_LEVEL CMake variable to decide whether headers should be marked SYSTEM or not. The PR fixes the resulting warnings for most headers, but the CesiumGltf headers still need to be addressed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Good for newcomers quality Improve code quality or encourage developer success
Projects
None yet
Development

No branches or pull requests

3 participants