This is an archive of the discontinued LLVM Phabricator instance.

Ground work for cuda-related checks in clang-tidy
Needs ReviewPublic

Authored by barcisz on Sep 7 2022, 10:14 AM.

Details

Summary

This diff adds support for adding CUDA checks to clang-tidy, as well as tests for those checks. This is in preparation for the checks introduced in D133804 and D133956, both of which were
developed by Meta (formerly Facebook). The cuda-unused

Diff Detail

Event Timeline

barcisz created this revision.Sep 7 2022, 10:14 AM
Herald added a project: Restricted Project. · View Herald TranscriptSep 7 2022, 10:14 AM
barcisz requested review of this revision.Sep 7 2022, 10:14 AM
Herald added a project: Restricted Project. · View Herald TranscriptSep 7 2022, 10:14 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript
tschuett added inline comments.
clang-tools-extra/clang-tidy/cuda/CudaTidyModule.cpp
26

Is Google a copy and paste error?

33

Is Google a copy and paste error?

ivanmurashko added inline comments.
clang-tools-extra/test/lit.cfg.py
19 ↗(On Diff #458497)

.cu is specified several times there. It looks like the change is not necessary at the line

barcisz updated this revision to Diff 458502.Sep 7 2022, 10:29 AM

Fixed a copy-paste error with Google->Cuda in CudaTidyModule.cpp

barcisz updated this revision to Diff 458504.Sep 7 2022, 10:31 AM

removed duplication of '.cu' in the lit config for clang-tidy tests

barcisz added inline comments.Sep 7 2022, 10:33 AM
clang-tools-extra/clang-tidy/cuda/CudaTidyModule.cpp
26

Done

33

Done

clang-tools-extra/test/lit.cfg.py
19 ↗(On Diff #458497)

Done

barcisz edited the summary of this revision. (Show Details)Sep 13 2022, 2:16 PM

@tschuett does it look alright now?

barcisz updated this revision to Diff 460027.Sep 14 2022, 3:52 AM

Better explanation of cuda-related flags in tests

barcisz updated this revision to Diff 460070.Sep 14 2022, 6:58 AM

Common documentation for cuda checks

What's with the .keep files. Will lit or sphinx fail if the cuda directory doesn't exist, if not I'd be happier removing them(they'd get removed once the first cuda check lands anyway).

clang-tools-extra/clang-tidy/cuda/CudaTidyModule.cpp
2

Google...

It may be easier/safer to use the add_new_check script to generate this, then just remove all the new check specific files.

clang-tools-extra/docs/ReleaseNotes.rst
98 ↗(On Diff #460070)

Put it in here.

152 ↗(On Diff #460070)

This line doesn't belong in the new checks portion of the release notes, just stick it in the general clang-tidy section.
Also maybe just say "Introduce the cuda module for checks specific to cuda code" and remove all the documentation below, That will be filled in when the first check lands.

clang-tools-extra/docs/clang-tidy/index.rst
143 ↗(On Diff #460070)

Unnecessary edit can be removed.

241 ↗(On Diff #460070)

Ditto.

clang-tools-extra/test/clang-tidy/checkers/Inputs/Headers/cuda/cuda-initializers.h
2

What's the purpose of this file here, do you envision many checks requiring to include this?

clang-tools-extra/test/clang-tidy/checkers/Inputs/Headers/cuda/cuda.h
4

This looks worrying as test cases don't run with a standard library

barcisz updated this revision to Diff 460374.Sep 15 2022, 5:16 AM
barcisz marked 2 inline comments as done.

Changes suggested by njames93

barcisz marked 5 inline comments as done.Sep 15 2022, 5:18 AM
barcisz updated this revision to Diff 460519.Sep 15 2022, 3:39 PM

Dummy definition for size_t

barcisz updated this revision to Diff 460520.Sep 15 2022, 3:43 PM

moved size_t definition to an stddef.h stub

njames93 added inline comments.Sep 17 2022, 6:50 AM
clang-tools-extra/test/clang-tidy/checkers/Inputs/Headers/stddef.h
12 ↗(On Diff #460520)

If this is all the file is used for, and it's only used for this one test file, can probably remove this header and inline the definition.
I also feel this definition may be fragile on certain platforms.

barcisz added inline comments.Sep 17 2022, 10:14 AM
clang-tools-extra/test/clang-tidy/checkers/Inputs/Headers/stddef.h
12 ↗(On Diff #460520)

I wanted to add it in a separate header for future proofness; as for the definition, I cannot really use uint64_t here since I cannot include the real stddef.h

aaron.ballman added inline comments.Sep 18 2022, 5:25 AM
clang-tools-extra/test/clang-tidy/checkers/Inputs/Headers/stddef.h
12 ↗(On Diff #460520)

using size_t = decltype(sizeof(0)); is a better way to spell this, and I'd probably inline it rather than add a new header file.

barcisz updated this revision to Diff 461561.Sep 20 2022, 7:09 AM

Inlined definition of size_t

LegalizeAdulthood resigned from this revision.Mar 29 2023, 8:20 AM