Configure clang-tidy job on CI

.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. 
  - 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/range_table.hpp

@@ -84,6 +84,7 @@ template <unsigned BLOCK_SIZE, storage::Ownership Ownership> class RangeTable
  unsigned block_counter = 0;
  BlockT block;
  unsigned block_sum = 0;
+ (void)block_sum;
  for (const unsigned l : lengths)
  {
  // first entry of a block: encode absolute offset

include/util/typedefs.hpp

@@ -50,8 +50,10 @@ struct duplicated_node
 };
 } // namespace tag
 using OSMNodeID = osrm::Alias<std::uint64_t, tag::osm_node_id>;
+// 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;
 }