Configure clang-tidy job on CI (#6261)

.clang-tidy

@@ -1,4 +1,85 @@
 ---
-Checks:          '-clang-analyzer-*,google-*,llvm-*,misc-*,readability-*,-google-build-explicit-make-pair,-google-explicit-constructor,-google-readability-braces-around-statements,-google-readability-casting,-google-readability-namespace-comments,-google-readability-function,-google-readability-todo,-google-runtime-int,-llvm-namespace-comment,-llvm-header-guard,-llvm-twine-local,-misc-argument-comment,-readability-braces-around-statements,-readability-identifier-naming'
+Checks: >
-...
+    bugprone-*,
+    -bugprone-narrowing-conversions,
+    -bugprone-easily-swappable-parameters,
+    -bugprone-branch-clone,
+    -bugprone-misplaced-widening-cast,
+    -bugprone-exception-escape,
+    -bugprone-implicit-widening-of-multiplication-result,
+    -bugprone-integer-division,
+    -bugprone-reserved-identifier,
+    -bugprone-macro-parentheses,
+    -bugprone-unhandled-self-assignment,
+    -bugprone-suspicious-missing-comma,
+    -bugprone-forward-declaration-namespace,
+    -bugprone-sizeof-expression,
+    -clang-analyzer-*,
+    -clang-diagnostic-unused-local-typedef,
+    -clang-diagnostic-deprecated-declarations,
+    google-*,
+    -google-build-explicit-make-pair,
+    -google-build-using-namespace,
+    -google-explicit-constructor,
+    -google-default-arguments,
+    -google-readability-braces-around-statements,
+    -google-readability-casting,
+    -google-readability-namespace-comments,
+    -google-readability-function,
+    -google-readability-todo,
+    -google-runtime-int,
+    -google-build-namespaces,
+    -google-global-names-in-headers,
+    -google-runtime-references,
+    -google-readability-function-size,
+    llvm-*,
+    -llvm-namespace-comment,
+    -llvm-qualified-auto,
+    -llvm-include-order,
+    -llvm-else-after-return,
+    -llvm-header-guard,
+    -llvm-twine-local,
+    misc-*,
+    -misc-argument-comment,
+    -misc-non-private-member-variables-in-classes,
+    -misc-unused-using-decls,
+    -misc-unconventional-assign-operator,
+    -misc-redundant-expression,
+    -misc-no-recursion,
+    -misc-misplaced-const,
+    -misc-definitions-in-headers,
+    -misc-unused-alias-decls,
+    -misc-unused-parameters,
+    readability-*,
+    -readability-avoid-const-params-in-decls,
+    -readability-braces-around-statements,
+    -readability-container-size-empty,
+    -readability-convert-member-functions-to-static,
+    -readability-const-return-type,
+    -readability-function-cognitive-complexity,
+    -readability-function-size,
+    -readability-identifier-naming,
+    -readability-implicit-bool-conversion,
+    -readability-magic-numbers,
+    -readability-else-after-return,
+    -readability-inconsistent-declaration-parameter-name,
+    -readability-isolate-declaration,
+    -readability-redundant-declaration,
+    -readability-uppercase-literal-suffix,
+    -readability-named-parameter,
+    -readability-qualified-auto,
+    -readability-suspicious-call-argument,
+    -readability-redundant-access-specifiers,
+    -readability-redundant-member-init,
+    -readability-static-definition-in-anonymous-namespace,
+    -readability-use-anyofallof,
+    -readability-simplify-boolean-expr,
+    -readability-make-member-function-const,
+    -readability-redundant-string-init,
+    -readability-non-const-parameter,
+    -readability-static-accessed-through-instance
+    
+WarningsAsErrors: '*'
+HeaderFilterRegex: '.*'
+
 

.github/PULL_REQUEST_TEMPLATE.md

@@ -9,7 +9,7 @@ If your PR is still work in progress please attach the relevant label.
 
  - [ ] CHANGELOG.md entry ([How to write a changelog entry](http://keepachangelog.com/en/1.0.0/#how))
  - [ ] update relevant [Wiki pages](https://github.com/Project-OSRM/osrm-backend/wiki)
- - [ ] add tests (see [testing documentation](https://github.com/Project-OSRM/osrm-backend/blob/master/docs/testing.md)
+ - [ ] add tests (see [testing documentation](https://github.com/Project-OSRM/osrm-backend/blob/master/docs/testing.md))
  - [ ] review
  - [ ] adjust for comments
  - [ ] cherry pick to release branch

.github/workflows/osrm-backend.yml

@@ -85,6 +85,16 @@ jobs:
             CLANG_VERSION: 5.0.0
             CUCUMBER_TIMEOUT: 60000
 
+          - name: clang-11.0-debug-clang-tidy
+            continue-on-error: false
+            node: 12
+            runs-on: ubuntu-20.04
+            BUILD_TOOLS: ON
+            BUILD_TYPE: Debug
+            CLANG_VERSION: 11.0.0
+            CUCUMBER_TIMEOUT: 60000
+            ENABLE_CLANG_TIDY: ON
+
           - name: mason-linux-debug-asan
             continue-on-error: false
             node: 12
@@ -365,6 +375,7 @@ jobs:
       CXXCOMPILER: ${{ matrix.CXXCOMPILER }}
       CXXFLAGS: ${{ matrix.CXXFLAGS }}
       ENABLE_ASSERTIONS: ${{ matrix.ENABLE_ASSERTIONS }}
+      ENABLE_CLANG_TIDY: ${{ matrix.ENABLE_CLANG_TIDY }} 
       ENABLE_COVERAGE: ${{ matrix.ENABLE_COVERAGE }}
       ENABLE_GLIBC_WORKAROUND: ${{ matrix.ENABLE_GLIBC_WORKAROUND }}
       ENABLE_MASON: ${{ matrix.ENABLE_MASON }}
@@ -490,7 +501,8 @@ jobs:
         pushd ${OSRM_BUILD_DIR}
         cmake .. -DCMAKE_BUILD_TYPE=${BUILD_TYPE} \
                  -DENABLE_MASON=${ENABLE_MASON:-OFF} \
-                 -DENABLE_ASSERTIONS=${ENABLE_ASSERTIONS:-OFF}} \
+                 -DENABLE_ASSERTIONS=${ENABLE_ASSERTIONS:-OFF} \
+                 -DENABLE_CLANG_TIDY=${ENABLE_CLANG_TIDY:-OFF} \
                  -DBUILD_SHARED_LIBS=${BUILD_SHARED_LIBS:-OFF} \
                  -DENABLE_COVERAGE=${ENABLE_COVERAGE:-OFF} \
                  -DENABLE_NODE_BINDINGS=${ENABLE_NODE_BINDINGS:-OFF} \

CHANGELOG.md

@@ -3,6 +3,7 @@
     - API:
       - FIXED: Fix inefficient osrm-routed connection handling [#6113](https://github.com/Project-OSRM/osrm-backend/pull/6113)
     - Build:
+      - CHANGED: Configure clang-tidy job on CI. [#6261](https://github.com/Project-OSRM/osrm-backend/pull/6261)
       - CHANGED: Use Github Actions for building container images [#6138](https://github.com/Project-OSRM/osrm-backend/pull/6138)
       - CHANGED: Upgrade Boost dependency to 1.70 [#6113](https://github.com/Project-OSRM/osrm-backend/pull/6113)
       - CHANGED: Upgrade Ubuntu CI builds to 20.04  [#6119](https://github.com/Project-OSRM/osrm-backend/pull/6119)

CMakeLists.txt

@@ -32,6 +32,18 @@ option(ENABLE_FUZZING "Fuzz testing using LLVM's libFuzzer" OFF)
 option(ENABLE_GOLD_LINKER "Use GNU gold linker if available" ON)
 option(ENABLE_NODE_BINDINGS "Build NodeJs bindings" OFF)
 option(ENABLE_GLIBC_WORKAROUND "Workaround GLIBC symbol exports" OFF)
+option(ENABLE_CLANG_TIDY "Enables clang-tidy checks" OFF)
+
+if (ENABLE_CLANG_TIDY)
+  find_program(CLANG_TIDY_COMMAND NAMES clang-tidy)
+  if(NOT CLANG_TIDY_COMMAND)
+    message(FATAL_ERROR "ENABLE_CLANG_TIDY is ON but clang-tidy is not found!")
+  else()
+    message(STATUS "Found clang-tidy at ${CLANG_TIDY_COMMAND}")
+    set(CMAKE_CXX_CLANG_TIDY "${CLANG_TIDY_COMMAND};--warnings-as-errors=*")
+  endif()
+endif()
+
 
 list(APPEND CMAKE_MODULE_PATH "${CMAKE_CURRENT_SOURCE_DIR}/cmake")
 
@@ -449,7 +461,7 @@ include_directories(SYSTEM ${VTZERO_INCLUDE_DIR})
 
 set(FLATBUFFERS_SRC_DIR "${CMAKE_CURRENT_SOURCE_DIR}/third_party/flatbuffers")
 set(FLATBUFFERS_INCLUDE_DIR "${CMAKE_CURRENT_SOURCE_DIR}/third_party/flatbuffers/include")
-include_directories(${FLATBUFFERS_INCLUDE_DIR})
+include_directories(SYSTEM ${FLATBUFFERS_INCLUDE_DIR})
 add_subdirectory(${FLATBUFFERS_SRC_DIR}
         ${CMAKE_CURRENT_BINARY_DIR}/flatbuffers-build
         EXCLUDE_FROM_ALL)

include/engine/datafacade/mmap_memory_allocator.hpp

@@ -22,7 +22,7 @@ namespace datafacade
 /**
  * This allocator uses file backed mmap memory block as the data location.
  */
-class MMapMemoryAllocator : public ContiguousBlockAllocator
+class MMapMemoryAllocator final : public ContiguousBlockAllocator
 {
   public:
     explicit MMapMemoryAllocator(const storage::StorageConfig &config);

include/engine/datafacade/process_memory_allocator.hpp

@@ -20,7 +20,7 @@ namespace datafacade
  * This class holds a unique_ptr to the memory block, so it
  * is auto-freed upon destruction.
  */
-class ProcessMemoryAllocator : public ContiguousBlockAllocator
+class ProcessMemoryAllocator final : public ContiguousBlockAllocator
 {
   public:
     explicit ProcessMemoryAllocator(const storage::StorageConfig &config);

include/engine/datafacade/shared_memory_allocator.hpp

@@ -20,7 +20,7 @@ namespace datafacade
  * Many SharedMemoryDataFacade objects can be created that point to the same shared
  * memory block.
  */
-class SharedMemoryAllocator : public ContiguousBlockAllocator
+class SharedMemoryAllocator final : public ContiguousBlockAllocator
 {
   public:
     explicit SharedMemoryAllocator(

include/engine/geospatial_query.hpp

@@ -467,11 +467,13 @@ template <typename RTreeT, typename DataFacadeT> class GeospatialQuery
         const auto forward_geometry = datafacade.GetUncompressedForwardGeometry(geometry_id);
 
         const auto forward_weight_offset =
+            // NOLINTNEXTLINE(bugprone-fold-init-type)
             std::accumulate(forward_weights.begin(),
                             forward_weights.begin() + data.fwd_segment_position,
                             EdgeWeight{0});
 
         const auto forward_duration_offset =
+            // NOLINTNEXTLINE(bugprone-fold-init-type)
             std::accumulate(forward_durations.begin(),
                             forward_durations.begin() + data.fwd_segment_position,
                             EdgeDuration{0});

include/engine/routing_algorithms/shortest_path_impl.hpp

@@ -304,6 +304,7 @@ InternalRouteResult shortestPathSearch(SearchEngineData<Algorithm> &engine_worki
                 {
                     BOOST_ASSERT(target_phantom.IsValidReverseTarget());
                     new_total_weight_to_reverse = new_total_weight_to_forward;
+                    // NOLINTNEXTLINE(bugprone-use-after-move)
                     packed_leg_to_reverse = std::move(packed_leg_to_forward);
                     new_total_weight_to_forward = INVALID_EDGE_WEIGHT;
 
@@ -354,6 +355,7 @@ InternalRouteResult shortestPathSearch(SearchEngineData<Algorithm> &engine_worki
         {
             bool forward_to_forward =
                 (new_total_weight_to_forward != INVALID_EDGE_WEIGHT) &&
+                // NOLINTNEXTLINE(bugprone-use-after-move)
                 packed_leg_to_forward.front() == source_phantom.forward_segment_id.id;
             bool reverse_to_forward =
                 (new_total_weight_to_forward != INVALID_EDGE_WEIGHT) &&

include/extractor/intersection_bearings_container.hpp

@@ -55,6 +55,7 @@ template <storage::Ownership Ownership> class IntersectionBearingsContainer
                        bearing_classes.end(),
                        bearing_counts.begin(),
                        [](const auto &bearings) { return bearings.getAvailableBearings().size(); });
+        // NOLINTNEXTLINE(bugprone-fold-init-type)
         auto total_bearings = std::accumulate(bearing_counts.begin(), bearing_counts.end(), 0);
         class_id_to_ranges_table = RangeTable<16>{bearing_counts};
 

include/guidance/motorway_handler.hpp

@@ -18,7 +18,7 @@ namespace guidance
 {
 
 // Intersection handlers deal with all issues related to intersections.
-class MotorwayHandler : public IntersectionHandler
+class MotorwayHandler final : public IntersectionHandler
 {
   public:
     MotorwayHandler(const util::NodeBasedDynamicGraph &node_based_graph,

include/guidance/roundabout_handler.hpp

@@ -35,7 +35,7 @@ struct RoundaboutFlags
 // The roundabout handler processes all roundabout related instructions.
 // It performs both the distinction between rotaries and roundabouts and
 // assigns appropriate entry/exit instructions.
-class RoundaboutHandler : public IntersectionHandler
+class RoundaboutHandler final : public IntersectionHandler
 {
   public:
     RoundaboutHandler(const util::NodeBasedDynamicGraph &node_based_graph,

include/guidance/turn_handler.hpp

@@ -23,7 +23,7 @@ namespace guidance
 {
 
 // Intersection handlers deal with all issues related to intersections.
-class TurnHandler : public IntersectionHandler
+class TurnHandler final : public IntersectionHandler
 {
   public:
     TurnHandler(const util::NodeBasedDynamicGraph &node_based_graph,

include/util/typedefs.hpp

@@ -50,8 +50,11 @@ struct duplicated_node
 };
 } // namespace tag
 using OSMNodeID = osrm::Alias<std::uint64_t, tag::osm_node_id>;
+// clang-tidy fires `bugprone-throw-keyword-missing` here for unknown reason
+// NOLINTNEXTLINE(bugprone-throw-keyword-missing)
 static_assert(std::is_pod<OSMNodeID>(), "OSMNodeID is not a valid alias");
 using OSMWayID = osrm::Alias<std::uint64_t, tag::osm_way_id>;
+// NOLINTNEXTLINE(bugprone-throw-keyword-missing)
 static_assert(std::is_pod<OSMWayID>(), "OSMWayID is not a valid alias");
 
 using DuplicatedNodeID = std::uint64_t;

scripts/error_on_dirty.sh

@@ -10,6 +10,7 @@ dirty=$(git ls-files --modified)
 if [[ $dirty ]]; then
     echo $MSG
     echo $dirty
+    git diff
     exit 1
 else
     exit 0

src/extractor/intersection/coordinate_extractor.cpp

@@ -892,6 +892,7 @@ CoordinateExtractor::PrepareLengthCache(const std::vector<util::Coordinate> &coo
     segment_distances.reserve(coordinates.size());
     segment_distances.push_back(0);
     // sentinel
+    // NOLINTNEXTLINE(bugprone-unused-return-value)
     std::find_if(std::next(std::begin(coordinates)),
                  std::end(coordinates),
                  [last_coordinate = coordinates.front(),

src/extractor/scripting_environment_lua.cpp

@@ -884,6 +884,7 @@ void Sol2ScriptingEnvironment::ProcessElements(
         case osmium::item_type::node:
         {
             const auto &node = static_cast<const osmium::Node &>(*entity);
+            // NOLINTNEXTLINE(bugprone-use-after-move)
             result_node.clear();
             if (local_context.has_node_function &&
                 (!node.tags().empty() || local_context.properties.call_tagless_node_function))
@@ -896,6 +897,7 @@ void Sol2ScriptingEnvironment::ProcessElements(
         case osmium::item_type::way:
         {
             const osmium::Way &way = static_cast<const osmium::Way &>(*entity);
+            // NOLINTNEXTLINE(bugprone-use-after-move)
             result_way.clear();
             if (local_context.has_way_function)
             {

src/nodejs/CMakeLists.txt

@@ -15,6 +15,9 @@ message(STATUS "Configuring node_osrm bindings for NodeJs ${NODEJS_VERSION}")
 
 add_nodejs_module(node_osrm node_osrm.cpp)
 set_target_properties(node_osrm PROPERTIES CXX_STANDARD 14)
+# TODO: we disable clang-tidy for this target, because it causes errors in third-party NodeJs related headers
+set_target_properties(node_osrm PROPERTIES CXX_CLANG_TIDY "")
+
 target_link_libraries(node_osrm osrm)
 
 # node_osrm artifacts in ${BINDING_DIR} to depend targets on

src/osrm/osrm.cpp

@@ -47,7 +47,7 @@ OSRM::OSRM(engine::EngineConfig &config)
         engine_ = std::make_unique<engine::Engine<MLD>>(config);
         break;
     default:
-        util::exception("Algorithm not implemented!");
+        throw util::exception("Algorithm not implemented!");
     }
 }
 OSRM::~OSRM() = default;

src/updater/updater.cpp

@@ -112,6 +112,7 @@ void checkWeightsConsistency(
         if (geometry_id.forward)
         {
             auto range = segment_data.GetForwardWeights(geometry_id.id);
+            // NOLINTNEXTLINE(bugprone-fold-init-type)
             EdgeWeight weight = std::accumulate(range.begin(), range.end(), EdgeWeight{0});
             if (weight > edge.data.weight)
             {
@@ -122,6 +123,7 @@ void checkWeightsConsistency(
         else
         {
             auto range = segment_data.GetReverseWeights(geometry_id.id);
+            // NOLINTNEXTLINE(bugprone-fold-init-type)
             EdgeWeight weight = std::accumulate(range.begin(), range.end(), EdgeWeight{0});
             if (weight > edge.data.weight)
             {
@@ -689,6 +691,7 @@ Updater::LoadAndUpdateEdgeExpandedGraph(std::vector<extractor::EdgeBasedEdge> &e
                 new_weight += weight;
             }
             const auto durations = segment_data.GetForwardDurations(geometry_id.id);
+            // NOLINTNEXTLINE(bugprone-fold-init-type)
             new_duration = std::accumulate(durations.begin(), durations.end(), EdgeWeight{0});
         }
         else
@@ -704,6 +707,7 @@ Updater::LoadAndUpdateEdgeExpandedGraph(std::vector<extractor::EdgeBasedEdge> &e
                 new_weight += weight;
             }
             const auto durations = segment_data.GetReverseDurations(geometry_id.id);
+            // NOLINTNEXTLINE(bugprone-fold-init-type)
             new_duration = std::accumulate(durations.begin(), durations.end(), EdgeWeight{0});
         }
         return std::make_tuple(new_weight, new_duration);

src/util/coordinate_calculation.cpp

@@ -386,7 +386,7 @@ double findClosestDistance(const std::vector<Coordinate> &lhs, const std::vector
             std::min(current_min, findClosestDistance(coordinate, rhs.begin(), rhs.end()));
         return false;
     };
-
+    // NOLINTNEXTLINE(bugprone-unused-return-value)
     std::find_if(std::begin(lhs), std::end(lhs), compute_minimum_distance_in_rhs);
     return current_min;
 }