Enable even more clang-tidy checks #6273

SiarheiFedartsou · 2022-07-02T09:58:56Z

Issue

Continuation of #6270 and #6261

Tasklist

CHANGELOG.md entry (How to write a changelog entry)
update relevant Wiki pages
add tests (see testing documentation)
review
adjust for comments
cherry pick to release branch

Requirements / Relations

Link any requirements here. Other pull requests this PR is based on?

SiarheiFedartsou · 2022-07-02T10:06:51Z

include/contractor/query_graph.hpp

@@ -15,7 +15,7 @@ namespace detail
 {
 template <storage::Ownership Ownership>
 using QueryGraph = util::StaticGraph<typename QueryEdge::EdgeData, Ownership>;
-}
+} // namespace detail


clang-tidy has a check which fires if we skip it somewhere, I fixed some of them, but didn't enable it, because there are tons of such places and probably worth doing it in separate PR

src/extractor/extractor.cpp

SiarheiFedartsou · 2022-07-02T10:49:08Z

@mjjbell please take a look when you have a chance 🙂

src/extractor/extractor.cpp

mjjbell · 2022-07-03T00:02:36Z

include/extractor/intersection_bearings_container.hpp

@@ -46,9 +46,9 @@ template <storage::Ownership Ownership> class IntersectionBearingsContainer
    IntersectionBearingsContainer &operator=(IntersectionBearingsContainer &&) = default;
    IntersectionBearingsContainer &operator=(const IntersectionBearingsContainer &) = default;

-    IntersectionBearingsContainer(std::vector<BearingClassID> node_to_class_id_,
+    IntersectionBearingsContainer(const std::vector<BearingClassID> &node_to_class_id,


What was the error message here? Here we guarantee a copy.
By having this as by-value, we can avoid a copy by doing two move operations.

It looks like the code isn't currently taking advantage of this, but it could.

osrm-backend/src/extractor/extractor.cpp

Line 863 in 3d2db20

IntersectionBearingsContainer{bearing_class_by_node_based_node,

It was performance-unnecessary-value-param it seems it is because this template <typename T> using Vector = util::ViewOrVector<T, Ownership>; does not have constructor which accepts std::vector<...>&&, but tbh I am not sure and decided to add NOLINT... for the time being to preserve a status quo here.

I see. The underlying container will depend on the ownership, so it can end up being std::vector. clang-tidy might not be able to figure it out.

Also, I can omit the linting comment on both my dev environments (Big Sur-clang14, Ubuntu 21.04-clang12) without error, so it does seem a bit unsure of its relevance.

Also, I can omit the linting comment on both my dev environments (Big Sur-clang14, Ubuntu 21.04-clang12) without error, so it does seem a bit unsure of its relevance.

It doesn't fire for me locally too, but does fire on CI.

Hmm, it must be a bug in that version. Even when I change the only invocation to be moving an r-value (see #6275), we get the same error. In any case, we'll need the nolint comment.

mjjbell · 2022-07-03T00:14:54Z

src/extractor/intersection/mergable_road_detector.cpp

@@ -419,10 +419,10 @@ bool MergableRoadDetector::HaveSameDirection(const NodeID intersection_node,
    // sampling to correctly weight longer segments in regression calculations
    const auto constexpr SAMPLE_INTERVAL = 5;
    coordinates_to_the_left = coordinate_extractor.SampleCoordinates(
-        std::move(coordinates_to_the_left), distance_to_extract, SAMPLE_INTERVAL);
+        coordinates_to_the_left, distance_to_extract, SAMPLE_INTERVAL);


mjjbell

👍

SiarheiFedartsou · 2022-07-04T16:04:53Z

@mjjbell could you merge please? I fixed merge conflicts, I guess it is the only problem which is left.

v5.27.0 - Changes from 5.26.0 - API: - ADDED: Add Flatbuffers support to NodeJS bindings. [Project-OSRM#6338](Project-OSRM#6338) - CHANGED: Add `data_version` field to responses of all services. [Project-OSRM#5387](Project-OSRM#5387) - FIXED: Use Boost.Beast to parse HTTP request. [Project-OSRM#6294](Project-OSRM#6294) - FIXED: Fix inefficient osrm-routed connection handling [Project-OSRM#6113](https://gihub.com/Project-OSRM/osrm-backend/pull/6113) - FIXED: Fix HTTP compression precedence [Project-OSRM#6113](Project-OSRM#6113) - NodeJS: - FIXED: Support `skip_waypoints` in Node bindings [Project-OSRM#6060](Project-OSRM#6060) - Misc: - ADDED: conanbuildinfo.json for easy reading of dependencies [Project-OSRM#6388](Project-OSRM#6388) - CHANGED: Improve performance of JSON rendering. Fix undefined behaviour in JSON numbers formatting. [Project-OSRM#6380](Project-OSRM#6380) - ADDED: Add timestamps for logs. [Project-OSRM#6375](Project-OSRM#6375) - CHANGED: Improve performance of map matching via getPathDistance optimization. [Project-OSRM#6378](Project-OSRM#6378) - CHANGED: Optimize RestrictionParser performance. [Project-OSRM#6344](Project-OSRM#6344) - ADDED: Support floats for speed value in traffic updates CSV. [Project-OSRM#6327](Project-OSRM#6327) - CHANGED: Use Lua 5.4 in Docker image. [Project-OSRM#6346](Project-OSRM#6346) - CHANGED: Remove redundant nullptr check. [Project-OSRM#6326](Project-OSRM#6326) - CHANGED: missing files list is included in exception message. [Project-OSRM#5360](Project-OSRM#5360) - CHANGED: Do not use deprecated Callback::Call overload in Node bindings. [Project-OSRM#6318](Project-OSRM#6318) - FIXED: Fix distance calculation consistency. [Project-OSRM#6315](Project-OSRM#6315) - FIXED: Fix performance issue after migration to sol2 3.3.0. [Project-OSRM#6304](Project-OSRM#6304) - CHANGED: Pass osm_node_ids by reference in osrm::updater::Updater class. [Project-OSRM#6298](Project-OSRM#6298) - FIXED: Fix bug with reading Set values from Lua scripts. [Project-OSRM#6285](Project-OSRM#6285) - FIXED: Bug in bicycle profile that caused exceptions if there is a highway=bicycle in the data. [Project-OSRM#6296](Project-OSRM#6296) - FIXED: Internal refactoring of identifier types used in data facade [Project-OSRM#6044](Project-OSRM#6044) - CHANGED: Update docs to reflect recent build and dependency changes [Project-OSRM#6383](Project-OSRM#6383) - Build: - REMOVED: Get rid of Mason. [Project-OSRM#6387](Project-OSRM#6387) - CHANGED: Use clang-format from CI base image. [Project-OSRM#6391](Project-OSRM#6391) - ADDED: Build Node bindings on Windows. [Project-OSRM#6334](Project-OSRM#6334) - ADDED: Configure cross-compilation for Apple Silicon. [Project-OSRM#6360](Project-OSRM#6360) - CHANGED: Use apt-get to install Clang on CI. [Project-OSRM#6345](Project-OSRM#6345) - CHANGED: Fix TBB in case of Conan + NodeJS build. [Project-OSRM#6333](Project-OSRM#6333) - CHANGED: Migrate to modern TBB version. [Project-OSRM#6300](Project-OSRM#6300) - CHANGED: Enable performance-move-const-arg clang-tidy check. [Project-OSRM#6319](Project-OSRM#6319) - CHANGED: Use the latest node on CI. [Project-OSRM#6317](Project-OSRM#6317) - CHANGED: Migrate Windows CI to GitHub Actions. [Project-OSRM#6312](Project-OSRM#6312) - ADDED: Add smoke test for Docker image. [Project-OSRM#6313](Project-OSRM#6313) - CHANGED: Update libosmium to version 2.18.0. [Project-OSRM#6303](Project-OSRM#6303) - CHANGED: Remove EXACT from find_package if using Conan. [Project-OSRM#6299](Project-OSRM#6299) - CHANGED: Configure Undefined Behaviour Sanitizer. [Project-OSRM#6290](Project-OSRM#6290) - CHANGED: Use Conan instead of Mason to install code dependencies. [Project-OSRM#6284](Project-OSRM#6284) - CHANGED: Migrate to C++17. Update sol2 to 3.3.0. [Project-OSRM#6279](Project-OSRM#6279) - CHANGED: Update macOS CI image to macos-11. [Project-OSRM#6286](Project-OSRM#6286) - CHANGED: Enable even more clang-tidy checks. [Project-OSRM#6273](Project-OSRM#6273) - CHANGED: Configure CMake to not build flatbuffers tests and samples. [Project-OSRM#6274](Project-OSRM#6274) - CHANGED: Enable more clang-tidy checks. [Project-OSRM#6270](Project-OSRM#6270) - CHANGED: Configure clang-tidy job on CI. [Project-OSRM#6261](Project-OSRM#6261) - CHANGED: Use Github Actions for building container images [Project-OSRM#6138](Project-OSRM#6138) - CHANGED: Upgrade Boost dependency to 1.70 [Project-OSRM#6113](Project-OSRM#6113) - CHANGED: Upgrade Ubuntu CI builds to 20.04 [Project-OSRM#6119](Project-OSRM#6119) - CHANGED: Make building osrm-routed optional [Project-OSRM#6144](Project-OSRM#6144) - FIXED: Run all unit tests in CI [Project-OSRM#5248](Project-OSRM#5248) - FIXED: Fix installation of Mason CMake and 32 bit CI build [Project-OSRM#6170](Project-OSRM#6170) - FIXED: Fixed Node docs generation check in CI. [Project-OSRM#6058](Project-OSRM#6058) - CHANGED: Docker build, enabled arm64 build layer [Project-OSRM#6172](Project-OSRM#6172) - CHANGED: Docker build, enabled apt-get update/install caching in separate layer for build phase [Project-OSRM#6175](Project-OSRM#6175) - FIXED: Bump CI complete meta job to ubuntu-20.04 [Project-OSRM#6323](Project-OSRM#6323) - CHANGED: Node packages are now scoped by @Project-OSRM [Project-OSRM#6386](Project-OSRM#6386) - Routing: - CHANGED: Lazily generate optional route path data [Project-OSRM#6045](Project-OSRM#6045) - FIXED: Completed support for no_entry and no_exit turn restrictions. [Project-OSRM#5988](Project-OSRM#5988) - ADDED: Add support for non-round-trips with a single fixed endpoint. [Project-OSRM#6050](Project-OSRM#6050) - FIXED: Improvements to maneuver override processing [Project-OSRM#6125](Project-OSRM#6125) - ADDED: Support snapping to multiple ways at an input location. [Project-OSRM#5953](Project-OSRM#5953) - FIXED: Fix snapping target locations to ways used in turn restrictions. [Project-OSRM#6339](Project-OSRM#6339) - ADDED: Support OSM traffic signal directions. [Project-OSRM#6153](Project-OSRM#6153) - FIXED: Ensure u-turn exists in intersection view. [Project-OSRM#6376](Project-OSRM#6376) - FIXED: Gracefully handle no-turn intersections in guidance processing. [Project-OSRM#6382](Project-OSRM#6382) - Profile: - CHANGED: Bicycle surface speeds [Project-OSRM#6212](Project-OSRM#6212) - Tools: - CHANGED: Do not generate intermediate .osrm file in osrm-extract. [Project-OSRM#6354](Project-OSRM#6354)

SiarheiFedartsou commented Jul 2, 2022

View reviewed changes

src/extractor/extractor.cpp Show resolved Hide resolved

SiarheiFedartsou marked this pull request as ready for review July 2, 2022 10:48

mjjbell reviewed Jul 3, 2022

View reviewed changes

SiarheiFedartsou requested a review from mjjbell July 3, 2022 11:25

mjjbell approved these changes Jul 4, 2022

View reviewed changes

SiarheiFedartsou force-pushed the sf-even-more-clang-tidy-checks branch from e213598 to 21944eb Compare July 4, 2022 16:04

Enable even more clang-tidy checks

099c2a3

SiarheiFedartsou force-pushed the sf-even-more-clang-tidy-checks branch from 21944eb to 099c2a3 Compare July 4, 2022 16:06

mjjbell merged commit 06b1b98 into Project-OSRM:master Jul 4, 2022

mattwigway pushed a commit to mattwigway/osrm-backend that referenced this pull request Jul 20, 2023

Enable even more clang-tidy checks (Project-OSRM#6273)

76e2fdb

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enable even more clang-tidy checks #6273

Enable even more clang-tidy checks #6273

SiarheiFedartsou commented Jul 2, 2022 •

edited

Loading

SiarheiFedartsou Jul 2, 2022

SiarheiFedartsou commented Jul 2, 2022

mjjbell Jul 3, 2022

SiarheiFedartsou Jul 3, 2022

mjjbell Jul 3, 2022

mjjbell Jul 4, 2022 •

edited

Loading

SiarheiFedartsou Jul 4, 2022

mjjbell Jul 4, 2022

mjjbell Jul 3, 2022

mjjbell left a comment

SiarheiFedartsou commented Jul 4, 2022

Enable even more clang-tidy checks #6273

Enable even more clang-tidy checks #6273

Conversation

SiarheiFedartsou commented Jul 2, 2022 • edited Loading

Issue

Tasklist

Requirements / Relations

SiarheiFedartsou Jul 2, 2022

Choose a reason for hiding this comment

SiarheiFedartsou commented Jul 2, 2022

mjjbell Jul 3, 2022

Choose a reason for hiding this comment

SiarheiFedartsou Jul 3, 2022

Choose a reason for hiding this comment

mjjbell Jul 3, 2022

Choose a reason for hiding this comment

mjjbell Jul 4, 2022 • edited Loading

Choose a reason for hiding this comment

SiarheiFedartsou Jul 4, 2022

Choose a reason for hiding this comment

mjjbell Jul 4, 2022

Choose a reason for hiding this comment

mjjbell Jul 3, 2022

Choose a reason for hiding this comment

mjjbell left a comment

Choose a reason for hiding this comment

SiarheiFedartsou commented Jul 4, 2022

SiarheiFedartsou commented Jul 2, 2022 •

edited

Loading

mjjbell Jul 4, 2022 •

edited

Loading