Merge pull request #1048 from CesiumGS/clang-tidy-cleanup

Fix clang-tidy warnings to make it actually useful
CesiumGS · Jan 13, 2025 · 132f87a · 132f87a
2 parents afc097e + 7eb9677
commit 132f87a
Show file tree

Hide file tree

Showing 447 changed files with 4,239 additions and 1,594 deletions.
diff --git a/.clang-tidy b/.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"]
diff --git a/.github/workflows/build.yml b/.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,18 @@ jobs:
           sudo swapon --show
       - name: Run clang-tidy
         run: |
-          cmake -B build -S . -DCMAKE_BUILD_TYPE=Debug
-          cmake --build build --target clang-tidy
+          echo `$CC --version | head -n 1`, `cmake --version | head -n 1`
+          cmake -B build -S . -DCMAKE_BUILD_TYPE=Debug -DCESIUM_CLANG_TIDY_USE_THREADS=4
+          cmake --build build --target clang-tidy > output.log
+      - name: List clang-tidy warnings & errors
+        if: ${{ !cancelled() }}
+        run: |
+          sed -n '/\(error\|warning\):/,/^$/p' output.log
+          # On macOS, the above doesn't work because the escaped pipe is not supported.
+          # Instead, use two commands:
+          #   sed -n '/error:/,/^$/p' output.log
+          #   sed -n '/warning:/,/^$/p' output.log
+
   Documentation:
     runs-on: ubuntu-22.04
     steps:

diff --git a/CMakeLists.txt b/CMakeLists.txt
@@ -116,6 +116,14 @@ cmake_dependent_option(
     OFF
 )
 
+cmake_dependent_option(
+    CESIUM_CLANG_TIDY_USE_THREADS
+    "Sets the number of threads for run-clang-tidy to use."
+    14
+    CESIUM_ENABLE_CLANG_TIDY
+    1
+)
+
 if(CESIUM_INSTALL_STATIC_LIBS OR CESIUM_INSTALL_HEADERS)
     foreach(PACKAGE ${PACKAGES_PUBLIC})
         string(REGEX REPLACE "\[.*\]" "" PACKAGE ${PACKAGE})

diff --git a/Cesium3DTiles/src/MetadataQuery.cpp b/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};
     }
   }
 

diff --git a/Cesium3DTiles/test/TestMetadataQuery.cpp b/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;
 

diff --git a/Cesium3DTilesContent/include/Cesium3DTilesContent/B3dmToGltfConverter.h b/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>

diff --git a/Cesium3DTilesContent/include/Cesium3DTilesContent/GltfConverterResult.h b/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>
 

diff --git a/Cesium3DTilesContent/include/Cesium3DTilesContent/GltfConverterUtility.h b/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

diff --git a/Cesium3DTilesContent/include/Cesium3DTilesContent/GltfConverters.h b/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>

diff --git a/Cesium3DTilesContent/include/Cesium3DTilesContent/PntsToGltfConverter.h b/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>

diff --git a/Cesium3DTilesContent/include/Cesium3DTilesContent/registerAllTileContentTypes.h b/Cesium3DTilesContent/include/Cesium3DTilesContent/registerAllTileContentTypes.h
@@ -1,4 +1,4 @@
-#include "Library.h"
+#include <Cesium3DTilesContent/Library.h>
 
 namespace Cesium3DTilesContent {
 

diff --git a/Cesium3DTilesContent/src/B3dmToGltfConverter.cpp b/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 =

diff --git a/Cesium3DTilesContent/src/BatchTableHierarchyPropertyValues.cpp b/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;