CesiumGS / cesium-native

Apache License 2.0
438 stars 215 forks source link

Add clang-tidy targets and CI check #1004

Open azrogers opened 1 day ago

azrogers commented 1 day ago
azrogers commented 1 day ago

After discussion with @kring, we're going to put aside completely resolving #460 for the moment, as adding a ton of casts to PropertyTable headers doesn't seem like it's necessarily the best use of resources right now. This means that CesiumGltf public headers will, for the moment, remain as SYSTEM and we won't receive build warnings or clang-tidy results for them. For future reference, making this change for CesiumGltf will require changing line 43 of CesiumGltf/CMakeLists.txt to the following:

cesium_target_include_directories(
    TARGET
        CesiumGltf
    PUBLIC
        ${CMAKE_CURRENT_LIST_DIR}/include/
        ${CMAKE_CURRENT_LIST_DIR}/generated/include
    PRIVATE
        ${CMAKE_CURRENT_LIST_DIR}/src
        ${CMAKE_CURRENT_LIST_DIR}/generated/src
)

With that out of the way, this should be good for review.

azrogers commented 1 day ago

Also, thank you to @lilleyse for the help here, especially with the changes to the code generation which were cherry-picked from his clang-tidy-3 branch.

kring commented 16 hours ago

I also looked very briefly through the list of new clang-tidy warnings (read: I look at the first couple), and saw this:

/usr/bin/clang-tidy-18 -p=/home/runner/work/cesium-native/cesium-native/build /home/runner/work/cesium-native/cesium-native/CesiumGeospatial/test/TestGlobeRectangle.cpp
/home/runner/work/cesium-native/cesium-native/CesiumGeospatial/test/TestGlobeRectangle.cpp:4:1: warning: included header catch.hpp is not used directly [misc-include-cleaner]
    4 | #include <catch2/catch.hpp>
      | ^~~~~~~~~~~~~~~~~~~~~~~~~~~
    5 | 
/home/runner/work/cesium-native/cesium-native/CesiumGeospatial/test/TestGlobeRectangle.cpp:9:1: warning: no header providing "TEST_CASE" is directly included [misc-include-cleaner]
    5 | 
    6 | using namespace CesiumGeospatial;
    7 | using namespace CesiumUtility;
    8 | 
    9 | TEST_CASE("GlobeRectangle::fromDegrees example") {
      | ^

Which isn't a good look, because catch2/catch.hpp is the header that provides TEST_CASE. Any idea why clang-tidy is confused here? Maybe we should be including individual catch2 headers?

lilleyse commented 4 hours ago

Maybe we should be including individual catch2 headers?

Yeah, in this case you would need to include <catch2/catch_test_macros.hpp> to silence the warning.

azrogers commented 3 hours ago

The Catch2 GitHub README and tutorial show using <catch2/catch_test_macros.hpp> instead of <catch2/catch.hpp> which we currently do, so I think that's the right move.

azrogers commented 5 minutes ago

First, cesium-native no longer builds on my system with MSVC 14.38:

[build] F:\cesium\cesium-unreal-samples\Plugins\cesium-unreal\extern\cesium-native\CesiumGltfReader\generated\src\GeneratedJsonHandlers.cpp(66,39): error C2220: the following warning is treated as an error [F:\cesium\cesium-unreal-samples\Plugins\cesium-unreal\extern\build\cesium-native\CesiumGltfReader\CesiumGltfReader.vcxproj] [build] F:\cesium\cesium-unreal-samples\Plugins\cesium-unreal\extern\cesium-native\CesiumGltfReader\generated\src\GeneratedJsonHandlers.cpp(66,39): warning C4455: 'operator ""s': literal suffix identifiers that do not start with an underscore are reserved [F:\cesium\cesium-unreal-samples\Plugins\cesium-unreal\extern\build\cesium-native\CesiumGltfReader\CesiumGltfReader.vcxproj]

This seems like it might be a bug with this older version of the compiler (https://developercommunity.visualstudio.com/t/warning-c4455-issued-when-using-standardized-liter/270349), but sadly Unreal requires us to use this version (or an even older one in some cases, 14.34). It's not obvious to me what changed in this branch to trigger this warning, though. Any ideas?

Looks like this comes down to a change made in the code generator from using namespace std::string_literals; in the GeneratedJsonHandlers to using std::string_literals::operator""s;. The latter is definitely more correct, but for whatever reason MSVC seems to have an issue with it and not with the former. I've reverted the change, which seems to have fixed this error.

azrogers commented 4 minutes ago

Unless I missed one, I believe I've taken care of all the review comments at the moment, whenever you get a chance to take another look @kring