Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[CPU] Enable clang-tidy-18 checks #28725

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

aobolensk
Copy link
Contributor

@aobolensk aobolensk commented Jan 29, 2025

Details:

  • Enable clang-tidy check on clang-18 (default for Ubuntu 24.04)
  • Fix newly appeared warnings

Tickets:

  • N/A

@github-actions github-actions bot added category: CI OpenVINO public CI github_actions Pull requests that update GitHub Actions code category: dockerfiles labels Jan 29, 2025
@aobolensk aobolensk force-pushed the clang-tidy-18 branch 2 times, most recently from 84932f6 to 5ca41cd Compare January 29, 2025 09:51
@github-actions github-actions bot added the category: build OpenVINO cmake script / infra label Jan 29, 2025
@aobolensk aobolensk force-pushed the clang-tidy-18 branch 7 times, most recently from 7e615e7 to f50c11d Compare January 29, 2025 15:49
@github-actions github-actions bot added the category: CPU OpenVINO CPU plugin label Jan 29, 2025
@aobolensk aobolensk marked this pull request as ready for review January 29, 2025 20:40
@aobolensk aobolensk requested review from a team as code owners January 29, 2025 20:40
@ilya-lavrenov ilya-lavrenov added this to the 2025.1 milestone Jan 30, 2025
@github-actions github-actions bot removed the github_actions Pull requests that update GitHub Actions code label Jan 30, 2025
@aobolensk aobolensk changed the title [CPU] Enable clang-tidy-18 checks on Ubuntu 24.04 [CPU] Enable clang-tidy-18 checks Jan 30, 2025
@@ -3,7 +3,7 @@
#

if(ENABLE_CLANG_TIDY)
set(CLANG_TIDY_REQUIRED_VERSION 15 CACHE STRING "clang-tidy version to use")
set(CLANG_TIDY_REQUIRED_VERSION 18 CACHE STRING "clang-tidy version to use")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ilya-lavrenov We want to bump up the version to 18. This is a default clang version on Ubuntu24. The reason - 18 version contains a feature misc-include-cleaner, which will allow us to fix most of the include mess and implicit include dependencies we have.
After we fix the includes, we will be able to implement an independent CI workflow for clang-tidy, which will only check the changed files independently and will not require to compile all the source files with clang-tidy enabled. Without fixing the includes those file-by-file checks may fail because of missing includes.
This will allow us to implement such workflow for all the architectures with very small execution time, basically it scales much better than building the whole project with clang-tidy.

The downside is that for clang-format we will have version 15 required, but for clang-tidy - version 18. And, for example, to install clang-tidy-18 on ubuntu22 it is required to add a ppa to apt sources. From my perspective these cons are not that important, since we already support Ubuntu24 where clang-tidy-18 is the default one.

What do you think?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't see a problem if we will use different versions of clang-family tools on different systems.
Once most of developers migrate to U24 as development machine, we can switch to clang-format-18

@EgorDuplensky
Copy link
Contributor

@dmitry-gorokhov Ready to merge
Please check the following comment for the details:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
category: build OpenVINO cmake script / infra category: CI OpenVINO public CI category: CPU OpenVINO CPU plugin category: dockerfiles
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants