Configure clang-tidy job on CI #6261

SiarheiFedartsou · 2022-06-16T13:39:57Z

Issue

Configures job on CI which runs clang-tidy on each pull request. Many of useful checks are disabled for the time being, because it was non-trivial to fix right in this PR, but it can be addressed later.

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-06-16T13:40:45Z

.github/workflows/osrm-backend.yml

+            runs-on: ubuntu-20.04
+            BUILD_TOOLS: ON
+            BUILD_TYPE: Debug
+            CLANG_VERSION: 11.0.0


11.0.0 is chosen because it is the latest version available in mason.

SiarheiFedartsou · 2022-06-16T13:41:56Z

scripts/error_on_dirty.sh

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


I use Apple Silicon mac, that's why script for formatting code doesn't work locally for me. It helped me to understand what CI wants from me to be formatted.

SiarheiFedartsou · 2022-06-17T14:22:40Z

@mjjbell may I ask you to take a look? Tagging you as a maintainer who reviewed recent merged PRs :)

mjjbell · 2022-06-21T21:46:48Z

@mjjbell may I ask you to take a look? Tagging you as a maintainer who reviewed recent merged PRs :)

Thanks, I'll take a look later in the week.

mjjbell · 2022-06-26T18:24:10Z

@SiarheiFedartsou can you include a changelog entry?

mjjbell

Thanks, this is already highlighting a few code smells.

Double-check the linting comment and add a changelog entry.

mjjbell · 2022-06-26T18:56:52Z

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)


Are these needed? I can omit these with clang 11.

Doesn't appear to match the description either.

Yes, for some reason it fires for me locally. I'll add a comment that it is intentional. 👍🏻

SiarheiFedartsou · 2022-06-27T07:07:47Z

Hi @mjjbell

Double-check the linting comment and add a changelog entry.

Is this entry in changelog not enough?

SiarheiFedartsou · 2022-06-27T07:20:24Z

Details

** continuous-integration/appveyor/pr ** — AppVeyor build succ

Well, I see that CI doesn't like it for some reason https://github.com/Project-OSRM/osrm-backend/runs/7062547493?check_suite_focus=true. Okay, I'll take a look.

mjjbell · 2022-06-27T08:41:34Z

Yes, my mistake. I saw the error and assumed it was missing.

I believe it's complaining about the white space at the end of the line.

osrm-backend/scripts/validate_changelog.js

Lines 20 to 21 in 41dda32

    
           if (!line.match(/\[#[0-9]+\]\(http.*\)$/)) { 
        
               line_errors.push("ERROR: changelog entries must end with an issue or PR link in Markdown format");

SiarheiFedartsou · 2022-06-16T13:43:45Z

.clang-tidy

-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-*,


Many of them are really useful, but fixing all of them is time consuming and it is better to do in separate PRs.

SiarheiFedartsou · 2022-06-27T07:05:28Z

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)


Yes, for some reason it fires for me locally. I'll add a comment that it is intentional. 👍🏻

SiarheiFedartsou · 2022-06-27T19:07:17Z

@mjjbell I've updated PR, please take a look again.

mjjbell

LGTM, thanks!

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 Jun 16, 2022

View reviewed changes

SiarheiFedartsou force-pushed the sf-clang-tidy2 branch from a5f1cfd to 2c5c3c3 Compare June 16, 2022 13:42

SiarheiFedartsou marked this pull request as ready for review June 16, 2022 13:44

SiarheiFedartsou force-pushed the sf-clang-tidy2 branch 2 times, most recently from e96b5cf to dc2f8b4 Compare June 16, 2022 13:47

mjjbell self-assigned this Jun 21, 2022

mjjbell requested changes Jun 26, 2022

View reviewed changes

SiarheiFedartsou commented Jun 27, 2022

View reviewed changes

Configure clang-tidy job on CI

de35af4

SiarheiFedartsou force-pushed the sf-clang-tidy2 branch from dc2f8b4 to de35af4 Compare June 27, 2022 18:44

SiarheiFedartsou requested a review from mjjbell June 27, 2022 18:45

mjjbell approved these changes Jun 27, 2022

View reviewed changes

mjjbell merged commit 5995317 into Project-OSRM:master Jun 27, 2022

This was referenced Jun 29, 2022

Enable more clang-tidy checks. #6270

Merged

Enable even more clang-tidy checks #6273

Merged

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

Configure clang-tidy job on CI (Project-OSRM#6261)

f3f7cc3

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Configure clang-tidy job on CI #6261

Configure clang-tidy job on CI #6261

SiarheiFedartsou commented Jun 16, 2022 •

edited

Loading

SiarheiFedartsou Jun 16, 2022

SiarheiFedartsou Jun 16, 2022

SiarheiFedartsou commented Jun 17, 2022

mjjbell commented Jun 21, 2022

mjjbell commented Jun 26, 2022

mjjbell left a comment

mjjbell Jun 26, 2022

SiarheiFedartsou Jun 27, 2022

SiarheiFedartsou commented Jun 27, 2022

SiarheiFedartsou commented Jun 27, 2022

mjjbell commented Jun 27, 2022

SiarheiFedartsou Jun 16, 2022

SiarheiFedartsou Jun 27, 2022

SiarheiFedartsou commented Jun 27, 2022

mjjbell left a comment

Configure clang-tidy job on CI #6261

Configure clang-tidy job on CI #6261

Conversation

SiarheiFedartsou commented Jun 16, 2022 • edited Loading

Issue

Tasklist

Requirements / Relations

SiarheiFedartsou Jun 16, 2022

Choose a reason for hiding this comment

SiarheiFedartsou Jun 16, 2022

Choose a reason for hiding this comment

SiarheiFedartsou commented Jun 17, 2022

mjjbell commented Jun 21, 2022

mjjbell commented Jun 26, 2022

mjjbell left a comment

Choose a reason for hiding this comment

mjjbell Jun 26, 2022

Choose a reason for hiding this comment

SiarheiFedartsou Jun 27, 2022

Choose a reason for hiding this comment

SiarheiFedartsou commented Jun 27, 2022

SiarheiFedartsou commented Jun 27, 2022

mjjbell commented Jun 27, 2022

SiarheiFedartsou Jun 16, 2022

Choose a reason for hiding this comment

SiarheiFedartsou Jun 27, 2022

Choose a reason for hiding this comment

SiarheiFedartsou commented Jun 27, 2022

mjjbell left a comment

Choose a reason for hiding this comment

SiarheiFedartsou commented Jun 16, 2022 •

edited

Loading