This is an archive of the discontinued LLVM Phabricator instance.

[clang-tidy] Add --fix-mode and --nolint-prefix options
Needs ReviewPublic

Authored by KyleFromKitware on Jan 31 2023, 9:15 AM.

Diff Detail

Event Timeline

Herald added a project: Restricted Project. · View Herald Transcript
KyleFromKitware requested review of this revision.Jan 31 2023, 9:15 AM
Herald added a project: Restricted Project. · View Herald TranscriptJan 31 2023, 9:15 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript
Eugene.Zelenko edited reviewers, added: aaron.ballman, hokein, gribozavr2, carlosgalvezp; removed: Restricted Project.Jan 31 2023, 11:02 AM

Fixed crash when warning doesn't have associated location.

Added tests and fixed assertion error in release build.

I just tried this out on a somewhat larger scale - I built all of CMake with this flag enabled, and it found a number of clang-tidy violations and added NOLINT comments to silence them. Worked like a charm.

Though I did find that it inserted more than one NOLINT comment for a templated function, probably due to multiple warnings being issued for the same violation.

gribozavr2 added inline comments.May 5 2023, 11:01 AM
clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp
58

Note that if the NOLINT is being inserted into a macro definition, we also need a backslash before the newline.

clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.h
245

Generally in Clang and adjacent projects we reserve the word "Type" to refer to C++ types. For other uses, we use a synonym - like "Kind" or "Mode".

I think "FixMode" will work well here, WDYT?

251

I think it would be better to use an enum class and avoid prefixing.

(There are a lot of plain enums in Clang because Clang was originally developed in C++03.)

clang-tools-extra/test/clang-tidy/infrastructure/fix-mode-nolint.cpp
2 ↗(On Diff #494031)

Could you add a second clang-tidy invocation over the fixed file to show that no warnings are produced? That would prove that the comments are inserted in the right place.

(here and in the other test)

7 ↗(On Diff #494031)

WDYT about making the comment style configurable? I think many people working in C++ will prefer //-style comments.

18 ↗(On Diff #494031)

Could you add more tests, specifically tests with macros?

  • A test where the problem is reported in the macro expansion, the warning is attached to the macro definition.
  • A test where the problem is reported in the macro expansion, but the warning is attached to the tokens in the macro argument.

The SourceLocation computations are quite subtle with macros so these cases are worth covering.

Note that when inserting NOLINTs into a macro definition, one has to use /* */-style comments.