Add clang-tidy targets and CI check

[azrogers]

• Added clang-tidy targets to CMake when building for a non-MSVC target. Currently clang-tidy is only set to warn, not raise errors, as there are too many issues to fix for one PR and it's not necessarily worth our time to do them all right now.
• Added the option CESIUM_ENABLE_CLANG_TIDY_ON_BUILD, which will run clang-tidy alongside every build command CMake runs. This slows down the build significantly, so it's disabled by default. However, this allows clang-tidy to be run even with an MSVC target. VSWhere is used on Windows to locate Visual Studio's copy of clang-tidy.
• Addressed part of Fix warnings in public headers #460, allowing warnings to be reported in public headers when cesium-native isn't being included in another CMake project. This has created a lot of errors around our use of int64_t versus size_t in PropertyTable-related classes, which has ended up requiring some pretty thorough changes to resolve.
• Added a CI action to run clang-tidy for each new commit.

[azrogers]

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]

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]

Looks good! I looked through all the non-generated changes, and only had a couple of small comments.

I also tried to build and run clang-tidy locally, though, and ran into some trouble.

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?

I also had trouble when compiling Cesium for Unreal's extern directory against this branch:

[cmake] CMake Error at cesium-native/CMakeLists.txt:110 (cmake_dependent_option):
[cmake] Unknown CMake command "cmake_dependent_option".

I was able to fix this by adding this line to cesium-native's CMakeList.txt, which I think is a reasonable (required?) thing to do

include(CMakeDependentOption)

But I don't know why this only shows up when building cesium-native for Unreal, not when building cesium-native itself. 🤷

[kring]

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]

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]

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]

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]

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

[azrogers]

The fact that clang-tidy produces a 21k line output for cesium-native is less than ideal. I think it's because each clang-tidy run for each file is independent, so the same headers end up getting checked and producing warnings over and over again. I feel like it would be nice to at least get all this outputted into a log file artifact to download and peruse, rather than almost crashing the browser tab trying to open it on GitHub, but I'm not sure whether that would really be all that useful.