Clang-tidy llvm checks in pre/post commit CI

Project Infrastructure
1

Hi all,

Do we run any clang-tidy checks on LLVM PRs either pre or post commit?

Looking further, seems it was disabled and there is an open issue to re-enable it here: enable clang-tidy by opt-in · Issue #367 · google/llvm-premerge-checks. Any idea if/when that will happen?

Thanks
Rahul

2

Specifically, clang-tidy has LLVM specific checks that would be nice to be enabled.

3

We definitely do not run any clang-tidy checks precommit and do not run any postcommit as far as I’m aware.

Enabling them precommit in a setup to the code formatting action would probably be the most convenient. It is just a matter of someone actually taking the time to set it up and to make sure that it is reliable enough at scale to be valuable.

4

Hi, I started experimenting with clang-tidy precommit CI. My initial goal is to first make it work on clang-tidy codebase because it’s already almost 100% compliant and maintainers have agreement that clang-tidy will benefit clang-tidy codebase. If it works smoothly, I’ll make a post and RFC about adding other projects to this CI.

My approach to implementation is to make it similar to code formatting CI, hopefully I will be able to deliver an MVP version in a couple of weeks.

5

For those of us who are not really familiar with clang-tidy, what are some of the key things it does that clang-format doesn’t?

6

Ideally in pre-merge I would think that a clang-tidy workflow would run before and after the proposed change and only display the new findings?
Also @vbvictor how would the UI look like? Would we be able to manage this like the CoPilot AI bot that leaves comments with suggestion to apply?

7

It’s a linter tool that operates on AST level, whereas clang-format runs on Lexer lever. It’s main goal to find bugs and violations of coding guidelines. For example we have a couple of LLVM-specific checks: twine-local, prefer-static-over-anonymous-namespace. It also can check functions/variables naming so that they confront with LLVM style:

8

Yes, It uses clang-tidy-diff.py script to only check new finding in CI.

For now I keep it out of scope and focus only on providing a basic message in one comment as clang-format does with steps to reproduce.
Integrating with comments is separate thing because in the past users complained that it was too noisy. We need an RFC for such changes.

9

Thanks. How easy is it to run clang-tidy on Windows? I develop in Visual Studio, which has built-in clang-format support. I don’t know whether there’s a clang-tidy equivalent though, which could add friction to Windows-developer workflows, if this becomes a mandatory step.

10

This part is the hardest to do because I don’t know github API and it will take some time and effort to learn and Implement this thing (unless someone will to help). I hope to incrementally deliver and maintain working CI.

11

@jh7370 here as some examples of findings that I applied to the MLIR codebase in the past:

Some checks in clang-tidy exists specifically for the LLVM standards:

These are all patches purely automatically generated by clang-tidy, there are also checkers in clang-tidy that are providing a diagnostic without a patch.

Also, here is an example of a commit adding a checker (100 lines of code) to clang-tidy specifically to help with automating the migration from a widely use API we’re deprecating: [clang-tidy] Add MLIR check for old op builder usage. (#149148) · llvm/llvm-project@3feb6f9 · GitHub

12

clang-format provides a patch though. Which is useful to handle version divergence (my local clang-format sometimes disagrees with CI).

Another question: sometimes clang-tidy suggestions can be incorrect and lead to code that does not compile for some reason, it may be expensive but can we try and validate the suggestions (by applying them and retesting with them)?

13

From top of my head, it seems to have support for clang-tidy, too. Also, steps to reproduce are just a couple cli scripts that comes in LLVM.exe installer and must work regardless of OS.

I think it will be a fairly long road till it becomes mandatory for whole LLVM, I hope to adapt it on “per-project” basis where each group of maintainers would agree to have clang-tidy in their CI.
As for clang-tools-extra/clang-tidy directory, I think we come to a universal agreement that we want to have clang-tidy [RFC] Create hardened .clang-tidy config for /clang-tidy directory - #12 by vbvictor

14

Note: even clang-format isn’t mandatory in the premerge CI today, it is still useful as-is. I would think clang-tidy suggestion can be valuable even without being mandatory.

15

clang-tidy usually gives fixit-its in output or descriptive message of how to fix the violation. So you could potentially even don’t run locally.

Those must be 100% bugs that we should fix. Our lit-tests we always check that code after fix-its compiles so those cases maybe not present in our test-suite. If there bugs on LLVM code I’m happy to triage and fix them with highest priority.

1 Like
16

Yes, AFAIK in LLVM almost everything is not mandatory so clang-tidy will also be non-mandatory.
It’s particularly useful in parts of LLVM that are beginner-friendly (like clang-tidy) because we have to deal with a lot of wrong variable naming style, etc.. Having clang-tidy in CI will hopefully save a ton of human resources.

1 Like
17

As the first iteration, It will look like this:

image
image1722×1136 143 KB

I’ll later clear output a bit removing unnecessary lines but the UI won’t dramatically change.

18

If I’m reading it correctly, this output shows a user how to generate the diff, but presumably you need to have built clang-tidy for it to work? Many developers will be working in environments where this might not be possible (e.g. cross-platform builds, where they don’t have the local target enabled). They may also not be familiar with using clang-tidy in general. Before this check is added, I think it’s essential that documentation be added to the LLVM GitHub documentation about the check and how to run clang-tidy locally.

Also, can I get a guarantee that the things clang-tidy warns about are 100% within the LLVM coding standards? It would be annoying to developers and reviewers if clang-tidy were to start warning about things that we don’t actually require in the standards, without agreement that it should be.

19

Thank you for feedback and concerns!

Most importantly, clang-tidy lint will not be available to broader public without a dedicated RFC. We will discuss pros/cons and overall strategy there.

For now as a MVP, clang-tidy will run only in clang-tools-extra/clang-tidy directory which contains clang-tidy source code. Developers of clang-tidy are expected to know how to use the tool they work on and clang-tidy maintainers agreed that premerge CI is beneficial for source code of clang-tidy.

To sum up, there won’t be any harm to LLVM subprojects.

The following answers to questions will be included in final RFC for clang-tidy:

clang-tidy and scripts for it are packaged with every release. For now, CI uses lastest stable release clang-tidy-20 which is available via atp/win-installer etc.. It should be as easy to install clang-tidy-20 as clang-20.
Moreover, you don’t need to reproduce all the instructions locally, you can just fix the warnings by following instructions in the published comment.

General documentation about clang-tidy is available here: Clang-Tidy — Extra Clang Tools 22.0.0git documentation, but I will work on a way to better present it for mass-users.

clang-tidy perform analysis based on .clang-tidy configs that are already inside llvm-project (most notably the one in root of LLVM). It’s expected that they reflect LLVM coding guidelines. Each of subproject in LLVM can have a subconfig with some rules enabled/disabled, e.g. projects like MLIR or CIR chose to have “stricter” configs and only maintainers of subprojects will decide what checks they want

20

Thanks, that seems reasonable to me.

I did some digging and for Windows developers who use Visual Studio, it looks like clang-tidy is available, just like clang-format, under the VS installation. It’s set up a bit differently though. See Using Clang-Tidy in Visual Studio | Microsoft Learn. It may be worth pointing at this documentation, but I’m not sure.

Thanks for the other responses, they seem fine to me.