salman-javed-nz updated this revision to Diff 372150.
salman-javed-nz added a comment.
Changes in this latest patch:
- `LineIsMarkedWithNOLINT()`: Moved `NOLINTBEGIN/END` aspects into a separate
function.
- `LineIsWithinNOLINTBEGIN()`: A `NOLINTBEGIN/END` region is only considered
valid if both the `BEGIN` and `END` markers are present. If the region is
malformed, the original warning will be raised.
Discussion points:
- Misuse of `NOLINTBEGIN/END` now always results in the original warning being
raised. However, a diagnostic directing the user to location of the mismatched
`BEGIN` and `END` markers is not implemented yet.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D108560/new/
https://reviews.llvm.org/D108560
Files:
clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp
clang-tools-extra/docs/ReleaseNotes.rst
clang-tools-extra/docs/clang-tidy/index.rst
clang-tools-extra/test/clang-tidy/infrastructure/Inputs/nolintbeginend/error_in_include.inc
clang-tools-extra/test/clang-tidy/infrastructure/Inputs/nolintbeginend/nolint_in_include.inc
clang-tools-extra/test/clang-tidy/infrastructure/nolintbeginend.cpp
Index: clang-tools-extra/test/clang-tidy/infrastructure/nolintbeginend.cpp
===================================================================
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/infrastructure/nolintbeginend.cpp
@@ -0,0 +1,90 @@
+// RUN: %check_clang_tidy %s google-explicit-constructor %t -- --header-filter=.* -system-headers -- -isystem %S/Inputs/nolintbeginend
+
+class A { A(int i); };
+// CHECK-MESSAGES: :[[@LINE-1]]:11: warning: single-argument constructors must be marked explicit
+
+// NOLINTEND
+class B1 { B1(int i); };
+// CHECK-MESSAGES: :[[@LINE-1]]:12: warning: single-argument constructors must be marked explicit
+
+// NOLINTBEGIN
+class B2 { B2(int i); };
+// NOLINTEND
+
+// NOLINTBEGIN
+// NOLINTEND
+// NOLINTBEGIN
+class B3 { B3(int i); };
+// NOLINTEND
+
+// NOLINTBEGIN
+// NOLINTEND
+class B4 { B4(int i); };
+// CHECK-MESSAGES: :[[@LINE-1]]:12: warning: single-argument constructors must be marked explicit
+
+// NOLINTBEGIN(google-explicit-constructor)
+class C1 { C1(int i); };
+// NOLINTEND(google-explicit-constructor)
+
+// NOLINTBEGIN(*)
+class C2 { C2(int i); };
+// NOLINTEND(*)
+
+// NOLINTBEGIN(some-other-check)
+class C3 { C3(int i); };
+// CHECK-MESSAGES: :[[@LINE-1]]:12: warning: single-argument constructors must be marked explicit
+// NOLINTEND(some-other-check)
+
+// NOLINTBEGIN(some-other-check, google-explicit-constructor)
+class C4 { C4(int i); };
+// NOLINTEND(some-other-check, google-explicit-constructor)
+
+// NOLINTBEGIN(some-other-check, google-explicit-constructor)
+// NOLINTEND(some-other-check)
+class C5 { C5(int i); };
+// NOLINTEND(google-explicit-constructor)
+
+// NOLINTBEGIN(some-other-check, google-explicit-constructor)
+// NOLINTEND(google-explicit-constructor)
+class C6 { C6(int i); };
+// CHECK-MESSAGES: :[[@LINE-1]]:12: warning: single-argument constructors must be marked explicit
+// NOLINTEND(some-other-check)
+
+// NOLINTBEGIN(google-explicit-constructor)
+// NOLINTBEGIN(some-other-check)
+class C7 { C7(int i); };
+// NOLINTEND(some-other-check)
+// NOLINTEND(google-explicit-constructor)
+
+// NOLINTBEGIN(not-closed-bracket-is-treated-as-skip-all
+class C8 { C8(int i); };
+// NOLINTEND(not-closed-bracket-is-treated-as-skip-all
+
+// NOLINTBEGIN without-brackets-skip-all, another-check
+class C9 { C9(int i); };
+// NOLINTEND without-brackets-skip-all, another-check
+
+#define MACRO(X) class X { X(int i); };
+
+MACRO(D1)
+// CHECK-MESSAGES: :[[@LINE-1]]:7: warning: single-argument constructors must be marked explicit
+
+// NOLINTBEGIN
+MACRO(D2)
+// NOLINTEND
+
+#define MACRO_NOARG class E { E(int i); };
+// NOLINTBEGIN
+MACRO_NOARG
+// NOLINTEND
+
+// NOLINTBEGIN
+class F { F(int i); };
+// CHECK-MESSAGES: :[[@LINE-1]]:11: warning: single-argument constructors must be marked explicit
+
+#include "error_in_include.inc"
+// CHECK-MESSAGES: error_in_include.inc:1:11: warning: single-argument constructors must be marked explicit
+
+#include "nolint_in_include.inc"
+
+// CHECK-MESSAGES: Suppressed 12 warnings (12 NOLINT)
Index: clang-tools-extra/test/clang-tidy/infrastructure/Inputs/nolintbeginend/nolint_in_include.inc
===================================================================
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/infrastructure/Inputs/nolintbeginend/nolint_in_include.inc
@@ -0,0 +1,3 @@
+// NOLINTBEGIN
+class H { H(int i); };
+// NOLINTEND
Index: clang-tools-extra/test/clang-tidy/infrastructure/Inputs/nolintbeginend/error_in_include.inc
===================================================================
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/infrastructure/Inputs/nolintbeginend/error_in_include.inc
@@ -0,0 +1 @@
+class G { G(int i); };
Index: clang-tools-extra/docs/clang-tidy/index.rst
===================================================================
--- clang-tools-extra/docs/clang-tidy/index.rst
+++ clang-tools-extra/docs/clang-tidy/index.rst
@@ -295,17 +295,19 @@
If a specific suppression mechanism is not available for a certain warning, or
its use is not desired for some reason, :program:`clang-tidy` has a generic
-mechanism to suppress diagnostics using ``NOLINT`` or ``NOLINTNEXTLINE``
-comments.
+mechanism to suppress diagnostics using ``NOLINT``, ``NOLINTNEXTLINE``, and
+``NOLINTBEGIN`` ... ``NOLINTEND`` comments.
The ``NOLINT`` comment instructs :program:`clang-tidy` to ignore warnings on the
*same line* (it doesn't apply to a function, a block of code or any other
language construct, it applies to the line of code it is on). If introducing the
comment in the same line would change the formatting in undesired way, the
``NOLINTNEXTLINE`` comment allows to suppress clang-tidy warnings on the *next
-line*.
+line*. The ``NOLINTBEGIN`` and ``NOLINTEND`` comments allow suppressing
+clang-tidy warnings on *multiple lines* (affecting all lines between the two
+comments).
-Both comments can be followed by an optional list of check names in parentheses
+All comments can be followed by an optional list of check names in parentheses
(see below for the formal syntax).
For example:
@@ -325,9 +327,16 @@
// Silence only the specified diagnostics for the next line
// NOLINTNEXTLINE(google-explicit-constructor, google-runtime-int)
Foo(bool param);
+
+ // Silence only the specified diagnostics for all lines between NOLINTBEGIN/END.
+ // NOLINTBEGIN(google-explicit-constructor, google-runtime-int)
+ Foo(short param);
+ Foo(long param);
+ // NOLINTEND(google-explicit-constructor, google-runtime-int)
};
-The formal syntax of ``NOLINT``/``NOLINTNEXTLINE`` is the following:
+The formal syntax of ``NOLINT``, ``NOLINTNEXTLINE``, and ``NOLINTBEGIN`` ...
+``NOLINTEND`` is the following:
.. parsed-literal::
@@ -345,11 +354,14 @@
lint-command:
**NOLINT**
**NOLINTNEXTLINE**
-
-Note that whitespaces between ``NOLINT``/``NOLINTNEXTLINE`` and the opening
-parenthesis are not allowed (in this case the comment will be treated just as
-``NOLINT``/``NOLINTNEXTLINE``), whereas in check names list (inside the
-parenthesis) whitespaces can be used and will be ignored.
+ **NOLINTBEGIN**
+ **NOLINTEND**
+
+Note that whitespaces between ``NOLINT``/``NOLINTNEXTLINE``/``NOLINTBEGIN``/``NOLINTEND``
+and the opening parenthesis are not allowed (in this case the comment will be
+treated just as ``NOLINT``/``NOLINTNEXTLINE``/``NOLINTBEGIN``/``NOLINTEND``),
+whereas in check names list (inside the parenthesis) whitespaces can be used and
+will be ignored.
.. _LibTooling: https://clang.llvm.org/docs/LibTooling.html
.. _How To Setup Tooling For LLVM: https://clang.llvm.org/docs/HowToSetupToolingForLLVM.html
Index: clang-tools-extra/docs/ReleaseNotes.rst
===================================================================
--- clang-tools-extra/docs/ReleaseNotes.rst
+++ clang-tools-extra/docs/ReleaseNotes.rst
@@ -67,7 +67,8 @@
Improvements to clang-tidy
--------------------------
-The improvements are...
+- Added support for `NOLINTBEGIN` ... `NOLINTEND` comments to suppress ClangTidy
+ warnings over multiple lines.
New checks
^^^^^^^^^^
Index: clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp
===================================================================
--- clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp
+++ clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp
@@ -342,6 +342,36 @@
: SM.getBufferDataIfLoaded(File);
}
+static bool LineIsWithinNOLINTBEGIN(llvm::StringRef Buffer, unsigned int Offset,
+ unsigned int DiagID,
+ const ClangTidyContext &Context) {
+ // Check if there's a NOLINTBEGIN on the previous lines.
+ SmallVector<StringRef> PrevLines;
+ Buffer.substr(0, Offset).rsplit('\n').first.split(PrevLines, '\n');
+ bool BeginNoLintFound = false;
+ for (auto I = PrevLines.rbegin(), E = PrevLines.rend(); I != E; ++I) {
+ if (IsNOLINTFound("NOLINTEND", *I, DiagID, Context))
+ return false;
+ if (IsNOLINTFound("NOLINTBEGIN", *I, DiagID, Context)) {
+ BeginNoLintFound = true;
+ break;
+ }
+ }
+ if (!BeginNoLintFound)
+ return false;
+
+ // Check that the NOLINTBEGIN is terminated correctly with a NOLINTEND
+ // on the following lines.
+ SmallVector<StringRef> FollowingLines;
+ Buffer.substr(Offset).split(FollowingLines, '\n');
+ for (const auto &Line : FollowingLines) {
+ if (IsNOLINTFound("NOLINTEND", Line, DiagID, Context))
+ return true;
+ }
+
+ return false;
+}
+
static bool LineIsMarkedWithNOLINT(const SourceManager &SM, SourceLocation Loc,
unsigned DiagID,
const ClangTidyContext &Context,
@@ -361,7 +391,11 @@
// Check if there's a NOLINTNEXTLINE on the previous line.
StringRef PrevLine =
Buffer->substr(0, Offset).rsplit('\n').first.rsplit('\n').second;
- return IsNOLINTFound("NOLINTNEXTLINE", PrevLine, DiagID, Context);
+ if (IsNOLINTFound("NOLINTNEXTLINE", PrevLine, DiagID, Context))
+ return true;
+
+ // Check if this line is within a NOLINTBEGIN ... NOLINTEND block.
+ return LineIsWithinNOLINTBEGIN(Buffer.getValue(), Offset, DiagID, Context);
}
static bool LineIsMarkedWithNOLINTinMacro(const SourceManager &SM,
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
