[PATCH] D142991: [clang-tidy] Add --fix-mode and --nolint-prefix options

[Dmitri Gribenko via Phabricator via cfe-commits]

gribozavr2 added inline comments.
Herald added a subscriber: PiotrZSL.


================
Comment at: clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp:58
+      (Lexer::getIndentationForLine(Loc, Loc.getManager()) + "/* " +
+       NoLintPrefix + "NOLINTNEXTLINE(" + Error.DiagnosticName + ") */\n")
+          .str());
----------------
Note that if the NOLINT is being inserted into a macro definition, we also need a backslash before the newline.


================
Comment at: clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.h:245
+/// Controls how clang-tidy applies fixes.
+enum FixType {
+  /// Only apply fix-its.
----------------
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?


================
Comment at: clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.h:251
+  /// Add NOLINT if fix-it is not available.
+  FT_FixItOrNoLint
+};
----------------
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.)


================
Comment at: clang-tools-extra/test/clang-tidy/infrastructure/fix-mode-nolint.cpp:2
+// RUN: grep -Ev "// *[A-Z-]+:" %s > %t.cpp
+// RUN: clang-tidy %t.cpp -checks='-*,modernize-use-trailing-return-type,readability-identifier-length' -fix -fix-mode=nolint -nolint-prefix='FIXME: ' -export-fixes=%t.yaml -- > %t.msg 2>&1
+// RUN: FileCheck -input-file=%t.cpp %s
----------------
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)


================
Comment at: clang-tools-extra/test/clang-tidy/infrastructure/fix-mode-nolint.cpp:7
+
+// CHECK: /* FIXME: NOLINTNEXTLINE(modernize-use-trailing-return-type) */
+// CHECK-NEXT: int func() {
----------------
WDYT about making the comment style configurable? I think many people working in C++ will prefer //-style comments.




================
Comment at: clang-tools-extra/test/clang-tidy/infrastructure/fix-mode-nolint.cpp:18
+  return i;
+}
----------------
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.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D142991/new/

https://reviews.llvm.org/D142991