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

Add support for a clang-tidy linter to CMake. Add a files utility function to P4CUtils.cmake. #4254

Merged
merged 1 commit into from
May 10, 2024

Conversation

fruffy
Copy link
Collaborator

@fruffy fruffy commented Nov 17, 2023

Add the ability to run clang-tidy across P4C files via CMake. This is not enabled yet for CI because clang-tidy is quite slow still.

However, one can still use it to run clang-tidy across one's files in bulk.

Supersedes #4253 if merged before it.

@fruffy fruffy requested a review from vlstill November 17, 2023 23:27
Copy link
Contributor

@vlstill vlstill left a comment

Choose a reason for hiding this comment

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

Generally looks good.

cmake/Linters.cmake Outdated Show resolved Hide resolved
cmake/Linters.cmake Outdated Show resolved Hide resolved
cmake/Linters.cmake Outdated Show resolved Hide resolved
cmake/Linters.cmake Outdated Show resolved Hide resolved
Copy link
Contributor

@vlstill vlstill left a comment

Choose a reason for hiding this comment

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

I've just now realized that you did not enable clang-tidy for any files. I think it would make more sense to enable it for the same set of files as cpplint and just run it from time to time. Otherwise there is not much point in having the infrastructure for it if it has no files to run on.

cmake/Linters.cmake Outdated Show resolved Hide resolved
Comment on lines +89 to +93
# TODO: Add files here once clang-tidy is fast enough.
# file(
# GLOB_RECURSE P4C_CLANG_TIDY_LINT_LIST FOLLOW_SYMLINKS
# )
# add_clang_tidy_files(${P4C_SOURCE_DIR} "${P4C_CLANG_TIDY_LINT_LIST}")
Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm, I understand that clang-tidy is slow, but having the target does not mean it will have to run. I think it would be best to add the same set as is used of cpplint and just let the programmers decide if they run it or not.

add_clang_tidy_files(${P4C_SOURCE_DIR} "${P4C_LINT_LIST}")

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The problem is that, until we have figured out this slowness issue, we can only apply clang-tidy selectively. If we added all the cpplint files the amount we would have to check would be overwhelming, when you just want to check a local set of files in your target.

I have been spot-checking files by adding add_clang_tidy_files(${P4C_SOURCE_DIR} "${P4C_LINT_LIST}") for a smaller subset of code I am interested in. I have not yet contributed these automated fixes because I am still tweaking rules etc.

Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm, this is also made worse by using the xargs which prevents running clang-tidy in parallel. This can be side-stepped by the following construction:

  • Use empty add_custom_target to add the actual target clang-tidy.
  • Use add_custom_command(TARGET clang-tidy POST_BUILD COMMAND ... for each file

But I still wonder if there is a better way of doing this then just manually adding files one wants to check (I would not expect clang-tidy to get significantly faster any time soon...). How long does it take to run clang-tidy over the all the CPPLINT-linted files?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The xargs is necessary because of a shell quirk. At some point the number of files as arguments grew too large.
There is a parallel runner but not sure it helps much when it takes ~10-30 second to analyze a single file.

I think the underlying problem is that clang-tidy checks system-files by default. Once we find a workaround for that it should be significantly faster.

Currently, checking all the CPPLINT files would a day at least.

@fruffy
Copy link
Collaborator Author

fruffy commented Jan 29, 2024

@vlstill This PR is ready. #4326 shows how it can be used. While we can not use clang-tidy at scale we can at least apply it in small increments. I would like to merge this PR before it starts to become too dated.

@fruffy fruffy added the infrastructure Topics related to code style and build and test infrastructure. label Feb 20, 2024
@fruffy fruffy requested a review from asl March 8, 2024 22:46
@fruffy
Copy link
Collaborator Author

fruffy commented Mar 8, 2024

@asl This might help in applying clang-tidy in broader strokes. If you have any advice on how to improve the performance of the analyser please let me know.

@asl
Copy link
Contributor

asl commented Mar 14, 2024

@fruffy By "performance" you mean that it is currently slow?

@fruffy
Copy link
Collaborator Author

fruffy commented Mar 14, 2024

Yes, clang-tidy requires many seconds to analyse a single file. This makes it a non-option for CI.

Copy link
Contributor

@vlstill vlstill left a comment

Choose a reason for hiding this comment

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

Let's get this in. In the end, it will be useful even if we can't run in in the P4C CI for now.

cmake/Linters.cmake Show resolved Hide resolved
@fruffy fruffy added this pull request to the merge queue May 10, 2024
Merged via the queue into main with commit 8edd039 May 10, 2024
16 of 17 checks passed
@fruffy fruffy deleted the fruffy/clang_tidy branch May 10, 2024 16:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
infrastructure Topics related to code style and build and test infrastructure.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants