Add clang-tidy for automatic linting

[andrewseidl]

Feature request

In similar spirit of #117, would be good to start using clang-tidy as a linter on the codebase to catch some errors, style violations, etc.

Arrow's setup would be a good place to start, which includes a CMake target to run the tool and a custom version of run-clang-tidy which supports ignoring some files.

[jrhemstad]

Duplicate https://github.com/rapidsai/cudf/issues/479

[vyasr]

@jrhemstad @harrism Now that we've merged rapidsai#6695 I think there is interest in enabling clang-tidy on the libcudf code base so I'm reopening this issue. If we'd prefer to consolidate discussion on #479 we can close this again and reopen that, but offhand it seems like #479 grew into a much more involved discussion about defining a style guide and enabling clang-tidy with some minimal configuration seems like a simpler goal to start with.

[harrism]

Agree! @teju85 is interested in having a shared cross-rapids clang-tidy config. This makes sense as we have more or less converged on a shared RAPIDS clang-format config.

[teju85]

Thank you Mark for adding me to this discussion. Yes, I'm certainly interested in a more uniform tidy config across RAPIDS!

BTW, we already have clang-tidy running as part of CI for cuml for almost a year now (though not super stringent, yet). cuML CI is here. However, when it was introduced, we were at clang v8.0.1 which wasn't particularly great with .cu/.cuh files! So, the tidy wrapper script had to take in a bunch of hacks :(. If someone can pick up this flow, improve and generalize it for RAPIDS-wide usage, that'd be awesome.

Finally, I'm not particularly attached to these scripts or a particular style (eg: google, cpp-core, etc) and will happily accept if there's a better solution for RAPIDS. However, I'd like for atleast all the key devs to be involved and their requirements heard before finalizing on a tidy style guide. Because in my personal opinion, tidying has a bigger benefit in terms of code uniformity and readability, especially for folks working across RAPIDS projects.

[teju85]

JFYI, there's a similar wrapper script in raft, but has not yet been added to CI workflow.

[vyasr]

@codereport would #10064 close this? We can always add more rules a la #10174 later as we decide that they are useful. Is there a reason we closed those PRs, or are they worth someone picking up?

[GregoryKimball]

I would like to pick up the work in #10064 after we've completed #12002

[vyasr]

As a first step I would prioritize finding a way to run clang-tidy in CI. PRs that fix clang-tidy errors are nice, but we inevitably accumulate more without CI checking for us.

[vyasr]

Based on what I found in #581, I think getting this working is going to first involve expending significant effort in ensuring that all of our dependencies compile with clang to start with. I know that cuML is already running clang-tidy, but they also have significant logic built around wrapping the clang-tidy invocation so I assume that they are also doing some preprocessing to only lint the files that they can compile. I could be wrong, though, and perhaps there's a simpler path here.

EDIT: It looks like cuML is skipping cu files altogether.

[harrism]

Clang can compile .cu files. Otherwise clangd intellisense wouldn't work.

[vyasr]

Clang can compile cu files, but there are various headers in libcu++/thrust/cuco/etc that compile under nvcc but not clang (for various reasons). I don't use Intellisense so I can't say exactly what is currently working and what isn't, perhaps it just skips TUs that don't compile.