Add clang-tidy

[Jacobfaib]

Description

Add a clang-tidy pass that runs over every created executable in CCCL. Run it via

$ cmake --build build --target tidy
[0/212] clang-tidy build/all-dev-debug/thrust/testing/thrust.cpp.cpp/address_stability.cu.cpp
...

The tidy target is only created for top-level builds. Crucially, the tidy target is lazy; if the host system has no clang-tidy, then an error is only emitted when you invoke --target tidy, not at configure-time. This allows seamless "opt-in" behavior.

Example of current errors generated:
clang_tidy_errors_sample.log

Please note that many errors (notably bugprone-reserved-identifier inside libcu++) can be locally silenced by a directory-local .clang-tidy, so please ignore them until the local .clang-tidy has been added.

Checklist

• New or existing tests cover these changes.
• The documentation is up to date with these changes.

[copy-pr-bot]

Auto-sync is disabled for draft pull requests in this repository. Workflows must be run manually.

Contributors can view more details about this message here.

[Jacobfaib]

Some follow-ups for (eventual) reviewers: readability-identifier-naming allows automating the naming scheme with clang-tidy. Each subproject follows roughly the same style guide, but not always (cudax/ for example is a bit all over the place).

I propose these checks are enabled in a follow-up, as it will be adding to an already massive diff in this PR.

[Jacobfaib]

Also which CI job is best suited for this? I notice there isn't really a unified "lint" job (other than pre-commit). clang-tidy works only on what it sees. We don't need to run it for every combination of compiler and OS that CCCL supports, but it should run in a configuration with as many optional features enabled as possible.

[miscco]

I am looking forward to clang-tidy, but we need to be really careful that we do not emit incorrect warnings.

We do have standard library components and need to account for that

[Jacobfaib]

We do have standard library components and need to account for that

The way this will be handled will be a separate .clang-tidy under libcudacxx/, I just haven't put it in yet. When emitting diagnostics, clang-tidy will find the closest .clang-tidy to the file that contains the code, so we can have different configurations for each package. For example, we will need to disable bugprone-reserved-identifier for it (actually, bugprone-reserved-identifier has an Invert option that flags names that are not prefixed with __).

[bernhardmgruber]

We do have standard library components and need to account for that

The way this will be handled will be a separate .clang-tidy under libcudacxx/, I just haven't put it in yet. When emitting diagnostics, clang-tidy will find the closest .clang-tidy to the file that contains the code, so we can have different configurations for each package. For example, we will need to disable bugprone-reserved-identifier for it (actually, bugprone-reserved-identifier has an Invert option that flags names that are not prefixed with __).

CUB and Thrust also use __ prefixes on some internal functions. At least I started using that. But a lot of other code uses a detail namespace. So we can actually run neither check before harmonizing to a single rule for internal functions :D

[jrhemstad]

Also which CI job is best suited for this? I notice there isn't really a unified "lint" job (other than pre-commit). clang-tidy works only on what it sees. We don't need to run it for every combination of compiler and OS that CCCL supports, but it should run in a configuration with as many optional features enabled as possible.

@alliepiper can you point @Jacobfaib in the right direction here?

[Jacobfaib]

CUB and Thrust also use __ prefixes on some internal functions. At least I started using that. But a lot of other code uses a detail namespace. So we can actually run neither check before harmonizing to a single rule for internal functions :D

So I misread clang-tidy when it said that it would match the closest .clang-tidy to a particular file. It matches the closest .clang-tidy to the source file, not the file that was included by the source file. So cuda/std/foo.h included by cccl/foo/bar/baz.cpp will never "match" against cccl/libcudacxx/.clang-tidy because clang-tidy finds cccl/.clang-tidy.

To fix this, I think we just disable bugprone-reserved-identifier entirely for now and then I can add in a custom check that looks up the .clang-tidy based on the location of the symbol (or just hard-code that anything in ::cuda::std is allowed __).

CUB and Thrust also use __ prefixes on some internal functions.

My 2 cents would be that only ::cuda::std is allowed the __. As low-level and special to NVIDIA as CUB and thrust are, they aren't really a "standard library" so detail namespace is appropriate. But will defer to others on this decision.

[miscco]

My 2 cents would be that only ::cuda::std is allowed the __. As low-level and special to NVIDIA as CUB and thrust are, they aren't really a "standard library" so detail namespace is appropriate. But will defer to others on this decision.

I believe that is correct, albeit we are starting to use effectively all of CUB in the PSTL CUDA backend, so technically ...

[alliepiper]

Also which CI job is best suited for this?

@Jacobfaib We should put this in a new CI job. We'll need that level of control. As a first step, I propose adding new ci/tidy_.sh scripts that use the existing common-ci-job infra to compile some new CMake presets, and some "tidy" jobs in ci/matrix.yaml that mirror the "build" job configs.

Take a look through the comments and docs in:

• ci/matrix.yaml (job defs, script locations and interfaces). See how the "build" job is setup (ignore the "build-lid0" etc variants, you won't need those), and use that as a template for defining a "tidy" job.
• CMakePresets.json is where you'll configure the tidy job configure, build, and (if needed?) ctest presets for the per-project, per-dialect builds.
• ci/build_libcudacxx.sh is the most straightforward build script to use as a template. You shouldn't need to mirror the complexity of say, build_cub.sh to get this working. We can look into splitting things up later if the runtime is bad enough.

---

Important: Use the override matrix in the ci/matrix.yaml file to test your new jobs without triggering a full 500+ job CI pipeline execution while iterating.

---

So:

1. New "tidy" job type def + pull_request/nightly/weekly jobs in ci/matrix.yaml (test with override matrix)
2. New config + build presets in CMakePresets.json
3. New ci/tidy_<project>.sh scripts per-project, following the build_libcudacxx.sh script as a template.

[Jacobfaib]

/ok to test

[Jacobfaib]

/ok to test

[Jacobfaib]

/ok to test

[Jacobfaib]

/ok to test

[Jacobfaib]

/ok to test

[Jacobfaib]

/ok to test

[Jacobfaib]

/ok to test

[Jacobfaib]

/ok to test

[Jacobfaib]

/ok to test

[Jacobfaib]

Per offline discussion, the consensus was that trying to go whole-hog with all checks enabled and fixes applied would be a nightmare to review/downright impossible.

Instead, we should merge a minimal passing version of this PR in ASAP to get the clang-tidy machinery itself into CI and the build system, then slowly ratchet up the enabled checks until we have everything enabled/fixed.

To whit, this PR now enables only a single check, performance-implicit-conversion-in-loop (https://github.com/NVIDIA/cccl/pull/8073/changes#diff-0534891fbc8b89b4c521057e7e8dff70d3cbeb822cc9991a3abad77848c289f8R23) (which CCCL seems to be passing).

[alliepiper]

We need to split this up into per-project scripts from this list to work with our change-detection logic that determines which areas of the CCCL need to be built and tested.

[fbusato]

totally fine if we disable the checks to add them in further PRs.
I strongly suggest further review of the cmake part because it touches several places.

[github-actions]

😬 CI Workflow Results

🟥 Finished in 3h 46m: Pass: 99%/446 | Total: 14d 22h | Max: 3h 46m | Hits: 66%/511977

See results here.