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

_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Reply via email to