Fix clang-tidy warnings to make it actually useful

.clang-tidy

@@ -17,7 +17,8 @@ Checks:
   - "bugprone-dynamic-static-initializers"
   - "-bugprone-easily-swappable-parameters"
   - "bugprone-empty-catch"
-  - "bugprone-exception-escape" 
+  # See https://github.com/CesiumGS/cesium-native/issues/348
+  #- "bugprone-exception-escape" 
   - "bugprone-fold-init-type"
   - "bugprone-forward-declaration-namespace"
   - "bugprone-forwarding-reference-overload"
@@ -36,7 +37,8 @@ Checks:
   - "bugprone-macro-repeated-side-effects"
   - "bugprone-misplaced-operator-in-strlen-in-alloc"
   - "bugprone-misplaced-pointer-arithmetic-in-alloc"
-  - "bugprone-misplaced-widening-cast"
+  # Produces a lot of results that are just extra-verbose - skipping for now
+  #- "bugprone-misplaced-widening-cast"
   - "bugprone-move-forwarding-reference"
   - "bugprone-multi-level-implicit-pointer-conversion"
   - "bugprone-multiple-new-in-one-expression"
@@ -53,7 +55,6 @@ Checks:
   - "bugprone-return-const-ref-from-parameter"
   - "bugprone-shared-ptr-array-mismatch"
   - "bugprone-signal-handler"
-  - "bugprone-signed-char-misuse"
   - "bugprone-sizeof-container"
   - "bugprone-sizeof-expression"
   - "bugprone-spuriously-wake-up-functions"
@@ -72,24 +73,64 @@ Checks:
   - "bugprone-suspicious-string-compare"
   - "bugprone-suspicious-stringview-data-usage"
   - "bugprone-swapped-arguments"
-  - "bugprone-switch-missing-default-case"
   - "bugprone-terminating-continue"
   - "bugprone-throw-keyword-missing"
   - "bugprone-too-small-loop-variable"
   - "bugprone-undefined-memory-manipulation"
   - "bugprone-undelegated-constructor"
-  - "bugprone-unhandled-exception-at-new"
+  #- "bugprone-unhandled-exception-at-new"
   - "bugprone-unique-ptr-array-mismatch"
   - "bugprone-unsafe-functions"
   - "bugprone-unused-local-non-trivial-variable"
   - "bugprone-unused-raii"
   - "bugprone-unused-return-value"
   - "bugprone-use-after-move"
   - "bugprone-virtual-near-miss"
+  - "cppcoreguidelines-avoid-capturing-lambda-coroutines"
+  - "cppcoreguidelines-avoid-goto"
+  - "cppcoreguidelines-avoid-reference-coroutine-parameters"
+  - "cppcoreguidelines-interfaces-global-init"
+  - "cppcoreguidelines-misleading-capture-default-by-value"
+  - "cppcoreguidelines-no-suspend-with-lock"
+  - "cppcoreguidelines-prefer-member-initializer"
+  - "cppcoreguidelines-pro-type-cstyle-cast"
+  - "cppcoreguidelines-pro-type-static-cast-downcast"
+  - "cppcoreguidelines-pro-type-union-access"
+  - "cppcoreguidelines-slicing"
+  - "google-runtime-int"
+  # We should enable this one at some point, but it produces a huge number of changes
+  # - "misc-const-correctness"
+  - "misc-misleading-bidrectional"
+  - "misc-misleading-identifier"
+  - "misc-misplaced-const"
+  - "misc-non-copyable-objects"
+  - "misc-redundant-expression"
+  - "misc-unused-parameters"
+  - "misc-unused-using-decls"
+  - "misc-use-anonymous-namespace"
+  - "misc-use-internal-linkage"
+  - "modernize-deprecated-headers"
+  #- "modernize-loop-convert"
+  - "modernize-make-shared"
+  - "modernize-make-unique"
+  - "modernize-min-max-use-initializer-list"
+  - "modernize-raw-string-literal"
+  - "modernize-redundant-void-arg"
+  - "modernize-replace-auto-ptr"
+  #- "modernize-type-traits"
+  #- "modernize-use-constraints"
+  - "modernize-use-emplace"
+  - "modernize-use-equals-default"
+  - "modernize-use-equals-delete"
+  - "modernize-use-nullptr"
+  - "modernize-use-starts-ends-with"
+  - "modernize-use-std-numbers"
+  #- "modernize-use-using"
+  - "performance-*"
+  - "-performance-enum-size"
   - "-readability-redundant-member-init"
 
-#WarningsAsErrors: "*"
-WarningsAsErrors: ""
+WarningsAsErrors: "*"
 FormatStyle: none
 CheckOptions:
   - key: readability-implicit-bool-conversion.AllowPointerConditions
@@ -99,7 +140,10 @@ CheckOptions:
   - key: modernize-use-auto.RemoveStars
     value: "true"
   - key: misc-include-cleaner.IgnoreHeaders
-    value: ".*cesium-async\\+\\+\\.h"
+    value: ".*cesium-async\\+\\+\\.h;.*bits\/.*.h"
+  - key: performance-move-const-arg.CheckTriviallyCopyableMove
+    value: "false"
 HeaderFilterRegex: ".*"
 HeaderFileExtensions: ["h"]
+ExcludeHeaderFilterRegex: ".*\\.ezvcpkg\/.*"
 ImplementationFileExtensions: ["cpp"]

.github/workflows/build.yml

@@ -17,14 +17,18 @@ jobs:
     steps:
       - name: Check out repository code
         uses: actions/checkout@v4
-      - name: Install ninja
-        uses: seanmiddleditch/gha-setup-ninja@master
-      - name: Install nasm
-        uses: ilammy/setup-nasm@v1
+      - name: Install latest ninja and cmake
+        uses: lukka/get-cmake@latest
       - name: ccache
         uses: hendrikmuhs/[email protected]
         with:
           key: ccache-ubuntu-24.04-clang-clang-tidy
+      - name: Install latest clang and clang-tidy
+        run: |
+          wget https://apt.llvm.org/llvm.sh
+          chmod +x llvm.sh
+          sudo ./llvm.sh 19
+          sudo apt-get install clang-tidy-19
       - name: Cache vcpkg artifacts
         uses: actions/cache@v4
         with:
@@ -35,8 +39,8 @@ jobs:
             vcpkg-ubuntu-24.04-clang
       - name: Set CC and CXX
         run: |
-          echo "CC=clang-18" >> "$GITHUB_ENV"
-          echo "CXX=clang++-18" >> "$GITHUB_ENV"
+          echo "CC=/usr/bin/clang-19" >> "$GITHUB_ENV"
+          echo "CXX=/usr/bin/clang++-19" >> "$GITHUB_ENV"
       - name: Make more swap space available
         run: |
           sudo swapoff -a
@@ -47,8 +51,13 @@ jobs:
           sudo swapon --show
       - name: Run clang-tidy
         run: |
+          echo `$CC --version | head -n 1`, `cmake --version | head -n 1`
           cmake -B build -S . -DCMAKE_BUILD_TYPE=Debug
-          cmake --build build --target clang-tidy
+          cmake --build build --target clang-tidy > output.log
+      - name: List clang-tidy warnings & errors
+        run: |
+          sed -n '/\(error\|warning\):/,/^$/p' output.log
+
   Documentation:
     runs-on: ubuntu-22.04
     steps:

Cesium3DTiles/src/MetadataQuery.cpp

@@ -1,4 +1,12 @@
+#include <Cesium3DTiles/Class.h>
+#include <Cesium3DTiles/ClassProperty.h>
+#include <Cesium3DTiles/MetadataEntity.h>
 #include <Cesium3DTiles/MetadataQuery.h>
+#include <Cesium3DTiles/Schema.h>
+
+#include <optional>
+#include <string>
+#include <utility>
 
 namespace Cesium3DTiles {
 
@@ -14,10 +22,8 @@ MetadataQuery::findFirstPropertyWithSemantic(
 
   const Cesium3DTiles::Class& klass = classIt->second;
 
-  for (auto it = entity.properties.begin(); it != entity.properties.end();
-       ++it) {
-    const std::pair<std::string, CesiumUtility::JsonValue>& property = *it;
-    auto propertyIt = klass.properties.find(property.first);
+  for (const auto& propertyValue : entity.properties) {
+    auto propertyIt = klass.properties.find(propertyValue.first);
     if (propertyIt == klass.properties.end())
       continue;
 
@@ -26,9 +32,9 @@ MetadataQuery::findFirstPropertyWithSemantic(
       return FoundMetadataProperty{
           classIt->first,
           classIt->second,
-          it->first,
+          propertyValue.first,
           propertyIt->second,
-          it->second};
+          propertyValue.second};
     }
   }
 

Cesium3DTiles/test/TestMetadataQuery.cpp

@@ -1,8 +1,14 @@
+#include <Cesium3DTiles/Class.h>
+#include <Cesium3DTiles/ClassProperty.h>
+#include <Cesium3DTiles/MetadataEntity.h>
 #include <Cesium3DTiles/MetadataQuery.h>
+#include <Cesium3DTiles/Schema.h>
+#include <CesiumUtility/JsonValue.h>
 
-#include <catch2/catch.hpp>
 #include <catch2/catch_test_macros.hpp>
 
+#include <optional>
+
 using namespace Cesium3DTiles;
 using namespace CesiumUtility;
 

Cesium3DTilesContent/include/Cesium3DTilesContent/B3dmToGltfConverter.h

@@ -1,7 +1,6 @@
 #pragma once
 
-#include "GltfConverterResult.h"
-
+#include <Cesium3DTilesContent/GltfConverterResult.h>
 #include <CesiumAsync/Future.h>
 #include <CesiumGltf/Model.h>
 #include <CesiumGltfReader/GltfReader.h>

Cesium3DTilesContent/include/Cesium3DTilesContent/GltfConverterResult.h

@@ -1,7 +1,6 @@
 #pragma once
 
-#include "Library.h"
-
+#include <Cesium3DTilesContent/Library.h>
 #include <CesiumGltf/Model.h>
 #include <CesiumUtility/ErrorList.h>
 

Cesium3DTilesContent/include/Cesium3DTilesContent/GltfConverterUtility.h

@@ -91,7 +91,7 @@ int32_t createAccessorInGltf(
     const int32_t bufferViewId,
     const int32_t componentType,
     const int64_t count,
-    const std::string type);
+    const std::string& type);
 
 /**
  * Applies the given relative-to-center (RTC) translation to the transforms of

Cesium3DTilesContent/include/Cesium3DTilesContent/GltfConverters.h

@@ -1,8 +1,7 @@
 #pragma once
 
-#include "Library.h"
-
 #include <Cesium3DTilesContent/GltfConverterResult.h>
+#include <Cesium3DTilesContent/Library.h>
 #include <CesiumAsync/Future.h>
 #include <CesiumAsync/IAssetAccessor.h>
 #include <CesiumGeometry/Axis.h>

Cesium3DTilesContent/include/Cesium3DTilesContent/PntsToGltfConverter.h

@@ -1,7 +1,6 @@
 #pragma once
 
-#include "GltfConverterResult.h"
-
+#include <Cesium3DTilesContent/GltfConverterResult.h>
 #include <CesiumAsync/Future.h>
 #include <CesiumGltf/Model.h>
 #include <CesiumGltfReader/GltfReader.h>

Cesium3DTilesContent/include/Cesium3DTilesContent/registerAllTileContentTypes.h

@@ -1,4 +1,4 @@
-#include "Library.h"
+#include <Cesium3DTilesContent/Library.h>
 
 namespace Cesium3DTilesContent {
 

Cesium3DTilesContent/src/B3dmToGltfConverter.cpp

@@ -2,9 +2,20 @@
 
 #include <Cesium3DTilesContent/B3dmToGltfConverter.h>
 #include <Cesium3DTilesContent/BinaryToGltfConverter.h>
+#include <Cesium3DTilesContent/GltfConverterResult.h>
 #include <Cesium3DTilesContent/GltfConverters.h>
+#include <CesiumAsync/Future.h>
 #include <CesiumGltf/ExtensionCesiumRTC.h>
-#include <CesiumUtility/Log.h>
+#include <CesiumGltfReader/GltfReader.h>
+#include <CesiumUtility/Assert.h>
+
+#include <fmt/format.h>
+#include <rapidjson/document.h>
+
+#include <cstddef>
+#include <cstdint>
+#include <span>
+#include <utility>
 
 namespace Cesium3DTilesContent {
 namespace {
@@ -158,6 +169,7 @@ rapidjson::Document parseFeatureTableJsonData(
   if (rtcIt != document.MemberEnd() && rtcIt->value.IsArray() &&
       rtcIt->value.Size() == 3 && rtcIt->value[0].IsNumber() &&
       rtcIt->value[1].IsNumber() && rtcIt->value[2].IsNumber()) {
+    CESIUM_ASSERT(result.model.has_value());
     // Add the RTC_CENTER value to the glTF as a CESIUM_RTC extension.
     rapidjson::Value& rtcValue = rtcIt->value;
     auto& cesiumRTC =

Cesium3DTilesContent/src/BatchTableHierarchyPropertyValues.cpp

@@ -3,6 +3,13 @@
 #include <CesiumUtility/Assert.h>
 
 #include <glm/common.hpp>
+#include <rapidjson/document.h>
+#include <rapidjson/rapidjson.h>
+
+#include <cstddef>
+#include <cstdint>
+#include <string>
+#include <vector>
 
 using namespace Cesium3DTilesContent::CesiumImpl;
 

Cesium3DTilesContent/src/BatchTableToGltfStructuralMetadata.cpp

@@ -2,22 +2,46 @@
 
 #include "BatchTableHierarchyPropertyValues.h"
 
+#include <CesiumGltf/Buffer.h>
+#include <CesiumGltf/BufferView.h>
+#include <CesiumGltf/Class.h>
+#include <CesiumGltf/ClassProperty.h>
 #include <CesiumGltf/ExtensionExtMeshFeatures.h>
 #include <CesiumGltf/ExtensionKhrDracoMeshCompression.h>
 #include <CesiumGltf/ExtensionModelExtStructuralMetadata.h>
+#include <CesiumGltf/FeatureId.h>
+#include <CesiumGltf/Mesh.h>
+#include <CesiumGltf/MeshPrimitive.h>
 #include <CesiumGltf/Model.h>
+#include <CesiumGltf/PropertyTable.h>
+#include <CesiumGltf/PropertyTableProperty.h>
 #include <CesiumGltf/PropertyType.h>
 #include <CesiumGltf/PropertyTypeTraits.h>
+#include <CesiumGltf/Schema.h>
 #include <CesiumUtility/Assert.h>
-#include <CesiumUtility/Log.h>
-
-#include <glm/glm.hpp>
+#include <CesiumUtility/ErrorList.h>
+#include <CesiumUtility/JsonValue.h>
+
+#include <fmt/format.h>
+#include <glm/common.hpp>
+#include <rapidjson/document.h>
+#include <rapidjson/rapidjson.h>
+#include <rapidjson/stringbuffer.h>
 #include <rapidjson/writer.h>
 
+#include <cstddef>
+#include <cstdint>
+#include <cstring>
 #include <limits>
 #include <map>
+#include <optional>
+#include <span>
+#include <string>
 #include <type_traits>
 #include <unordered_set>
+#include <utility>
+#include <variant>
+#include <vector>
 
 using namespace CesiumGltf;
 using namespace Cesium3DTilesContent::CesiumImpl;
@@ -387,7 +411,7 @@ struct CompatibleTypes {
    * This is helpful for when a property contains a sentinel value as non-null
    * data; the sentinel value can then be removed from consideration.
    */
-  void removeSentinelValues(CesiumUtility::JsonValue value) noexcept {
+  void removeSentinelValues(const CesiumUtility::JsonValue& value) noexcept {
     if (value.isNumber()) {
       // Don't try to use string as sentinels for numbers.
       _canUseNullStringSentinel = false;
@@ -406,7 +430,7 @@ struct CompatibleTypes {
       _canUseZeroSentinel = false;
       _canUseNegativeOneSentinel = false;
 
-      auto stringValue = value.getString();
+      const auto& stringValue = value.getString();
       if (stringValue == "null") {
         _canUseNullStringSentinel = false;
       }
@@ -708,7 +732,7 @@ void updateExtensionWithJsonStringProperty(
       // Because serialized string json will add double quotations in the
       // buffer which is not needed by us, we will manually add the string to
       // the buffer
-      const auto& rapidjsonStr = it->IsNull() ? *noDataValue : it->GetString();
+      const auto& rapidjsonStr = it->GetString();
       rapidjsonStrBuffer.Reserve(it->GetStringLength());
       for (rapidjson::SizeType j = 0; j < it->GetStringLength(); ++j) {
         rapidjsonStrBuffer.PutUnsafe(rapidjsonStr[j]);
@@ -791,6 +815,7 @@ void updateExtensionWithJsonScalarProperty(
 
   for (int64_t i = 0; i < propertyTable.count; ++i, ++p, ++it) {
     if (it->IsNull()) {
+      CESIUM_ASSERT(noDataValue.has_value());
       *p = *noDataValue;
     } else {
       *p = static_cast<T>(it->template Get<TRapidJson>());