Enable even more clang-tidy checks by SiarheiFedartsou · Pull Request #6273 · Project-OSRM/osrm-backend

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

 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

    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

    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

Uh oh!

Enable even more clang-tidy checks#6273

Enable even more clang-tidy checks#6273
mjjbell merged 1 commit intoProject-OSRM:masterfrom
SiarheiFedartsou:sf-even-more-clang-tidy-checks

SiarheiFedartsou commented Jul 2, 2022 •

edited

Loading

Uh oh!

SiarheiFedartsou Jul 2, 2022

Uh oh!

Uh oh!

SiarheiFedartsou commented Jul 2, 2022

Uh oh!

Uh oh!

mjjbell Jul 3, 2022

Uh oh!

SiarheiFedartsou Jul 3, 2022

Uh oh!

mjjbell Jul 3, 2022

Uh oh!

mjjbell Jul 4, 2022 •

edited

Loading

Uh oh!

SiarheiFedartsou Jul 4, 2022

Uh oh!

mjjbell Jul 4, 2022

Uh oh!

mjjbell Jul 3, 2022

Uh oh!

mjjbell left a comment

Uh oh!

SiarheiFedartsou commented Jul 4, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Uh oh!

Conversation

SiarheiFedartsou commented Jul 2, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Issue

Tasklist

Requirements / Relations

Uh oh!

SiarheiFedartsou Jul 2, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

SiarheiFedartsou commented Jul 2, 2022

Uh oh!

Uh oh!

mjjbell Jul 3, 2022

Choose a reason for hiding this comment

Uh oh!

SiarheiFedartsou Jul 3, 2022

Choose a reason for hiding this comment

Uh oh!

mjjbell Jul 3, 2022

Choose a reason for hiding this comment

Uh oh!

mjjbell Jul 4, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SiarheiFedartsou Jul 4, 2022

Choose a reason for hiding this comment

Uh oh!

mjjbell Jul 4, 2022

Choose a reason for hiding this comment

Uh oh!

mjjbell Jul 3, 2022

Choose a reason for hiding this comment

Uh oh!

mjjbell left a comment

Choose a reason for hiding this comment

Uh oh!

SiarheiFedartsou commented Jul 4, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

SiarheiFedartsou commented Jul 2, 2022 •

edited

Loading

mjjbell Jul 4, 2022 •

edited

Loading