Skip to content

Commit bf6b0d1

Browse files
carlosgalvezpcarlosgalvezp
committed
[clang-tidy] Support globbing in NOLINT* expressions
To simplify suppressing warnings (for example, for when multiple check aliases are enabled). The globbing format reuses the same code as for globbing when enabling checks, so the semantics and behavior is identical. Differential Revision: https://reviews.llvm.org/D111208
1 parent b1ce454 commit bf6b0d1

11 files changed

+249
-94
lines changed

‎clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp

Lines changed: 34 additions & 48 deletions
Original file line numberDiff line numberDiff line change
@@ -35,6 +35,7 @@
3535
#include "llvm/Support/FormatVariadic.h"
3636
#include "llvm/Support/Regex.h"
3737
#include <tuple>
38+
#include <utility>
3839
#include <vector>
3940
using namespace clang;
4041
using namespace tidy;
@@ -321,11 +322,11 @@ void ClangTidyDiagnosticConsumer::finalizeLastError() {
321322

322323
static bool isNOLINTFound(StringRef NolintDirectiveText, StringRef CheckName,
323324
StringRef Line, size_t *FoundNolintIndex = nullptr,
324-
bool *SuppressionIsSpecific = nullptr) {
325+
StringRef *FoundNolintChecksStr = nullptr) {
325326
if (FoundNolintIndex)
326327
*FoundNolintIndex = StringRef::npos;
327-
if (SuppressionIsSpecific)
328-
*SuppressionIsSpecific = false;
328+
if (FoundNolintChecksStr)
329+
*FoundNolintChecksStr = StringRef();
329330

330331
size_t NolintIndex = Line.find(NolintDirectiveText);
331332
if (NolintIndex == StringRef::npos)
@@ -345,18 +346,13 @@ static bool isNOLINTFound(StringRef NolintDirectiveText, StringRef CheckName,
345346
if (BracketEndIndex != StringRef::npos) {
346347
StringRef ChecksStr =
347348
Line.substr(BracketIndex, BracketEndIndex - BracketIndex);
348-
// Allow disabling all the checks with "*".
349-
if (ChecksStr != "*") {
350-
// Allow specifying a few check names, delimited with comma.
351-
SmallVector<StringRef, 1> Checks;
352-
ChecksStr.split(Checks, ',', -1, false);
353-
llvm::transform(Checks, Checks.begin(),
354-
[](StringRef S) { return S.trim(); });
355-
if (llvm::find(Checks, CheckName) == Checks.end())
356-
return false;
357-
if (SuppressionIsSpecific)
358-
*SuppressionIsSpecific = true;
359-
}
349+
if (FoundNolintChecksStr)
350+
*FoundNolintChecksStr = ChecksStr;
351+
// Allow specifying a few checks with a glob expression, ignoring
352+
// negative globs (which would effectively disable the suppression).
353+
GlobList Globs(ChecksStr, /*KeepNegativeGlobs=*/false);
354+
if (!Globs.contains(CheckName))
355+
return false;
360356
}
361357
}
362358

@@ -388,28 +384,27 @@ static ClangTidyError createNolintError(const ClangTidyContext &Context,
388384
return Error;
389385
}
390386

391-
static Optional<ClangTidyError>
392-
tallyNolintBegins(const ClangTidyContext &Context, const SourceManager &SM,
393-
StringRef CheckName, SmallVector<StringRef> Lines,
394-
SourceLocation LinesLoc,
395-
SmallVector<SourceLocation> &SpecificNolintBegins,
396-
SmallVector<SourceLocation> &GlobalNolintBegins) {
397-
// Keep a running total of how many NOLINT(BEGIN...END) blocks are active.
387+
static Optional<ClangTidyError> tallyNolintBegins(
388+
const ClangTidyContext &Context, const SourceManager &SM,
389+
StringRef CheckName, SmallVector<StringRef> Lines, SourceLocation LinesLoc,
390+
SmallVector<std::pair<SourceLocation, StringRef>> &NolintBegins) {
391+
// Keep a running total of how many NOLINT(BEGIN...END) blocks are active, as
392+
// well as the bracket expression (if any) that was used in the NOLINT
393+
// expression.
398394
size_t NolintIndex;
399-
bool SuppressionIsSpecific;
400-
auto List = [&]() -> SmallVector<SourceLocation> * {
401-
return SuppressionIsSpecific ? &SpecificNolintBegins : &GlobalNolintBegins;
402-
};
395+
StringRef NolintChecksStr;
403396
for (const auto &Line : Lines) {
404397
if (isNOLINTFound("NOLINTBEGIN", CheckName, Line, &NolintIndex,
405-
&SuppressionIsSpecific)) {
398+
&NolintChecksStr)) {
406399
// Check if a new block is being started.
407-
List()->emplace_back(LinesLoc.getLocWithOffset(NolintIndex));
400+
NolintBegins.emplace_back(std::make_pair(
401+
LinesLoc.getLocWithOffset(NolintIndex), NolintChecksStr));
408402
} else if (isNOLINTFound("NOLINTEND", CheckName, Line, &NolintIndex,
409-
&SuppressionIsSpecific)) {
403+
&NolintChecksStr)) {
410404
// Check if the previous block is being closed.
411-
if (!List()->empty()) {
412-
List()->pop_back();
405+
if (!NolintBegins.empty() &&
406+
NolintBegins.back().second == NolintChecksStr) {
407+
NolintBegins.pop_back();
413408
} else {
414409
// Trying to close a nonexistent block. Return a diagnostic about this
415410
// misuse that can be displayed along with the original clang-tidy check
@@ -432,42 +427,33 @@ lineIsWithinNolintBegin(const ClangTidyContext &Context,
432427
StringRef TextAfterDiag) {
433428
Loc = SM.getExpansionRange(Loc).getBegin();
434429
SourceLocation FileStartLoc = SM.getLocForStartOfFile(SM.getFileID(Loc));
430+
SmallVector<std::pair<SourceLocation, StringRef>> NolintBegins;
435431

436432
// Check if there's an open NOLINT(BEGIN...END) block on the previous lines.
437433
SmallVector<StringRef> PrevLines;
438434
TextBeforeDiag.split(PrevLines, '\n');
439-
SmallVector<SourceLocation> SpecificNolintBegins;
440-
SmallVector<SourceLocation> GlobalNolintBegins;
441-
auto Error =
442-
tallyNolintBegins(Context, SM, CheckName, PrevLines, FileStartLoc,
443-
SpecificNolintBegins, GlobalNolintBegins);
435+
auto Error = tallyNolintBegins(Context, SM, CheckName, PrevLines,
436+
FileStartLoc, NolintBegins);
444437
if (Error) {
445438
SuppressionErrors.emplace_back(Error.getValue());
446-
return false;
447439
}
448-
bool WithinNolintBegin =
449-
!SpecificNolintBegins.empty() || !GlobalNolintBegins.empty();
440+
bool WithinNolintBegin = !NolintBegins.empty();
450441

451442
// Check that every block is terminated correctly on the following lines.
452443
SmallVector<StringRef> FollowingLines;
453444
TextAfterDiag.split(FollowingLines, '\n');
454445
Error = tallyNolintBegins(Context, SM, CheckName, FollowingLines, Loc,
455-
SpecificNolintBegins, GlobalNolintBegins);
446+
NolintBegins);
456447
if (Error) {
457448
SuppressionErrors.emplace_back(Error.getValue());
458-
return false;
459449
}
460450

461451
// The following blocks were never closed. Return diagnostics for each
462452
// instance that can be displayed along with the original clang-tidy check
463453
// that the user was attempting to suppress.
464-
for (const auto NolintBegin : SpecificNolintBegins) {
465-
auto Error = createNolintError(Context, SM, NolintBegin, true);
466-
SuppressionErrors.emplace_back(Error);
467-
}
468-
for (const auto NolintBegin : GlobalNolintBegins) {
469-
auto Error = createNolintError(Context, SM, NolintBegin, true);
470-
SuppressionErrors.emplace_back(Error);
454+
for (const auto NolintBegin : NolintBegins) {
455+
SuppressionErrors.emplace_back(
456+
createNolintError(Context, SM, NolintBegin.first, true));
471457
}
472458

473459
return WithinNolintBegin && SuppressionErrors.empty();

‎clang-tools-extra/clang-tidy/GlobList.cpp

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -42,13 +42,14 @@ static llvm::Regex consumeGlob(StringRef &GlobList) {
4242
return llvm::Regex(RegexText);
4343
}
4444

45-
GlobList::GlobList(StringRef Globs) {
45+
GlobList::GlobList(StringRef Globs, bool KeepNegativeGlobs /* =true */) {
4646
Items.reserve(Globs.count(',') + 1);
4747
do {
4848
GlobListItem Item;
4949
Item.IsPositive = !consumeNegativeIndicator(Globs);
5050
Item.Regex = consumeGlob(Globs);
51-
Items.push_back(std::move(Item));
51+
if (Item.IsPositive || KeepNegativeGlobs)
52+
Items.push_back(std::move(Item));
5253
} while (!Globs.empty());
5354
}
5455

‎clang-tools-extra/clang-tidy/GlobList.h

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -29,7 +29,10 @@ class GlobList {
2929
///
3030
/// An empty \p Globs string is interpreted as one glob that matches an empty
3131
/// string.
32-
GlobList(StringRef Globs);
32+
///
33+
/// \p KeepNegativeGlobs a bool flag indicating whether to keep negative
34+
/// globs from \p Globs or not. When false, negative globs are simply ignored.
35+
GlobList(StringRef Globs, bool KeepNegativeGlobs = true);
3336

3437
/// Returns \c true if the pattern matches \p S. The result is the last
3538
/// matching glob's Positive flag.

‎clang-tools-extra/docs/ReleaseNotes.rst

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -67,6 +67,9 @@ The improvements are...
6767
Improvements to clang-tidy
6868
--------------------------
6969

70+
- Added support for globbing in `NOLINT*` expressions, to simplify suppressing
71+
multiple warnings in the same line.
72+
7073
- Added support for `NOLINTBEGIN` ... `NOLINTEND` comments to suppress
7174
Clang-Tidy warnings over multiple lines.
7275

‎clang-tools-extra/docs/clang-tidy/index.rst

Lines changed: 32 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -173,11 +173,11 @@ An overview of all the command-line options:
173173
errors were found. If compiler errors have
174174
attached fix-its, clang-tidy will apply them as
175175
well.
176-
--fix-notes -
177-
If a warning has no fix, but a single fix can
178-
be found through an associated diagnostic note,
179-
apply the fix.
180-
Specifying this flag will implicitly enable the
176+
--fix-notes -
177+
If a warning has no fix, but a single fix can
178+
be found through an associated diagnostic note,
179+
apply the fix.
180+
Specifying this flag will implicitly enable the
181181
'--fix' flag.
182182
--format-style=<string> -
183183
Style for formatting code around applied fixes:
@@ -308,7 +308,9 @@ clang-tidy warnings on *multiple lines* (affecting all lines between the two
308308
comments).
309309

310310
All comments can be followed by an optional list of check names in parentheses
311-
(see below for the formal syntax).
311+
(see below for the formal syntax). The list of check names supports globbing,
312+
with the same format and semantics as for enabling checks. Note: negative globs
313+
are ignored here, as they would effectively re-activate the warning.
312314

313315
For example:
314316

@@ -324,15 +326,39 @@ For example:
324326
// Silence only the specified checks for the line
325327
Foo(double param); // NOLINT(google-explicit-constructor, google-runtime-int)
326328

329+
// Silence all checks from the `google` module
330+
Foo(bool param); // NOLINT(google*)
331+
332+
// Silence all checks ending with `-avoid-c-arrays`
333+
int array[10]; // NOLINT(*-avoid-c-arrays)
334+
327335
// Silence only the specified diagnostics for the next line
328336
// NOLINTNEXTLINE(google-explicit-constructor, google-runtime-int)
329337
Foo(bool param);
330338

339+
// Silence all checks from the `google` module for the next line
340+
// NOLINTNEXTLINE(google*)
341+
Foo(bool param);
342+
343+
// Silence all checks ending with `-avoid-c-arrays` for the next line
344+
// NOLINTNEXTLINE(*-avoid-c-arrays)
345+
int array[10];
346+
331347
// Silence only the specified checks for all lines between the BEGIN and END
332348
// NOLINTBEGIN(google-explicit-constructor, google-runtime-int)
333349
Foo(short param);
334350
Foo(long param);
335351
// NOLINTEND(google-explicit-constructor, google-runtime-int)
352+
353+
// Silence all checks from the `google` module for all lines between the BEGIN and END
354+
// NOLINTBEGIN(google*)
355+
Foo(bool param);
356+
// NOLINTEND(google*)
357+
358+
// Silence all checks ending with `-avoid-c-arrays` for all lines between the BEGIN and END
359+
// NOLINTBEGIN(*-avoid-c-arrays)
360+
int array[10];
361+
// NOLINTEND(*-avoid-c-arrays)
336362
};
337363

338364
The formal syntax of ``NOLINT``, ``NOLINTNEXTLINE``, and ``NOLINTBEGIN`` ...

‎clang-tools-extra/test/clang-tidy/infrastructure/nolint.cpp

Lines changed: 28 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
// REQUIRES: static-analyzer
2-
// RUN: %check_clang_tidy %s google-explicit-constructor,clang-diagnostic-unused-variable,clang-analyzer-core.UndefinedBinaryOperatorResult %t -- -extra-arg=-Wunused-variable -- -I%S/Inputs/nolint
2+
// RUN: %check_clang_tidy %s google-explicit-constructor,clang-diagnostic-unused-variable,clang-analyzer-core.UndefinedBinaryOperatorResult,modernize-avoid-c-arrays,cppcoreguidelines-avoid-c-arrays %t -- -extra-arg=-Wunused-variable -- -I%S/Inputs/nolint
33

44
#include "trigger_warning.h"
55
void I(int& Out) {
@@ -17,17 +17,20 @@ class B { B(int i); }; // NOLINT
1717
class C { C(int i); }; // NOLINT(for-some-other-check)
1818
// CHECK-MESSAGES: :[[@LINE-1]]:11: warning: single-argument constructors must be marked explicit
1919

20-
class C1 { C1(int i); }; // NOLINT(*)
20+
class C1 { C1(int i); }; // NOLINT()
21+
// CHECK-MESSAGES: :[[@LINE-1]]:12: warning: single-argument constructors must be marked explicit
22+
23+
class C2 { C2(int i); }; // NOLINT(*)
2124

22-
class C2 { C2(int i); }; // NOLINT(not-closed-bracket-is-treated-as-skip-all
25+
class C3 { C3(int i); }; // NOLINT(not-closed-bracket-is-treated-as-skip-all
2326

24-
class C3 { C3(int i); }; // NOLINT(google-explicit-constructor)
27+
class C4 { C4(int i); }; // NOLINT(google-explicit-constructor)
2528

26-
class C4 { C4(int i); }; // NOLINT(some-check, google-explicit-constructor)
29+
class C5 { C5(int i); }; // NOLINT(some-check, google-explicit-constructor)
2730

28-
class C5 { C5(int i); }; // NOLINT without-brackets-skip-all, another-check
31+
class C6 { C6(int i); }; // NOLINT without-brackets-skip-all, another-check
2932

30-
class C6 { C6(int i); }; // NOLINTNEXTLINE doesn't get misconstrued as a NOLINT
33+
class C7 { C7(int i); }; // NOLINTNEXTLINE doesn't get misconstrued as a NOLINT
3134
// CHECK-MESSAGES: :[[@LINE-1]]:12: warning: single-argument constructors must be marked explicit
3235

3336
void f() {
@@ -51,4 +54,21 @@ MACRO_NOLINT
5154
#define DOUBLE_MACRO MACRO(H) // NOLINT
5255
DOUBLE_MACRO
5356

54-
// CHECK-MESSAGES: Suppressed 13 warnings (13 NOLINT)
57+
class D1 { D1(int x); }; // NOLINT(google*)
58+
class D2 { D2(int x); }; // NOLINT(*explicit-constructor)
59+
class D3 { D3(int x); }; // NOLINT(*explicit*)
60+
class D4 { D4(int x); }; // NOLINT(-explicit-constructor)
61+
// CHECK-MESSAGES: :[[@LINE-1]]:12: warning: single-argument constructors must be marked explicit
62+
class D5 { D5(int x); }; // NOLINT(google*,-google*)
63+
class D6 { D6(int x); }; // NOLINT(*,-google*)
64+
65+
int array1[10];
66+
// CHECK-MESSAGES: :[[@LINE-1]]:1: warning: do not declare C-style arrays, use std::array<> instead [cppcoreguidelines-avoid-c-arrays,modernize-avoid-c-arrays]
67+
68+
int array2[10]; // NOLINT(cppcoreguidelines-avoid-c-arrays)
69+
// CHECK-MESSAGES: :[[@LINE-1]]:1: warning: do not declare C-style arrays, use std::array<> instead [modernize-avoid-c-arrays]
70+
71+
int array3[10]; // NOLINT(cppcoreguidelines-avoid-c-arrays,modernize-avoid-c-arrays)
72+
int array4[10]; // NOLINT(*-avoid-c-arrays)
73+
74+
// CHECK-MESSAGES: Suppressed 23 warnings (23 NOLINT)

‎clang-tools-extra/test/clang-tidy/infrastructure/nolintbeginend-begin-global-end-specific.cpp

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -11,3 +11,15 @@ class A { A(int i); };
1111
// CHECK: :[[@LINE-6]]:4: error: unmatched 'NOLIN
1212
// CHECK: TEND' comment without a previous 'NOLIN
1313
// CHECK: TBEGIN' comment [clang-tidy-nolint]
14+
15+
// NOLINTBEGIN
16+
class B { B(int i); };
17+
// NOLINTEND(*)
18+
19+
// Note: the expected output has been split over several lines so that clang-tidy
20+
// does not see the "no lint" suppression comment and mistakenly assume it
21+
// is meant for itself.
22+
// CHECK: :[[@LINE-6]]:11: warning: single-argument constructors must be marked explicit
23+
// CHECK: :[[@LINE-6]]:4: error: unmatched 'NOLIN
24+
// CHECK: TEND' comment without a previous 'NOLIN
25+
// CHECK: TBEGIN' comment [clang-tidy-nolint]

‎clang-tools-extra/test/clang-tidy/infrastructure/nolintbeginend-begin-specific-end-global.cpp

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -11,3 +11,15 @@ class A { A(int i); };
1111
// CHECK: :[[@LINE-6]]:4: error: unmatched 'NOLIN
1212
// CHECK: TEND' comment without a previous 'NOLIN
1313
// CHECK: TBEGIN' comment [clang-tidy-nolint]
14+
15+
// NOLINTBEGIN(*)
16+
class B { B(int i); };
17+
// NOLINTEND
18+
19+
// Note: the expected output has been split over several lines so that clang-tidy
20+
// does not see the "no lint" suppression comment and mistakenly assume it
21+
// is meant for itself.
22+
// CHECK: :[[@LINE-6]]:11: warning: single-argument constructors must be marked explicit
23+
// CHECK: :[[@LINE-6]]:4: error: unmatched 'NOLIN
24+
// CHECK: TEND' comment without a previous 'NOLIN
25+
// CHECK: TBEGIN' comment [clang-tidy-nolint]

‎clang-tools-extra/test/clang-tidy/infrastructure/nolintbeginend-mismatched-check-names.cpp

Lines changed: 21 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -16,3 +16,24 @@ auto Num = (unsigned int)(-1);
1616
// CHECK: :[[@LINE-10]]:4: error: unmatched 'NOLIN
1717
// CHECK: TEND' comment without a previous 'NOLIN
1818
// CHECK: TBEGIN' comment [clang-tidy-nolint]
19+
20+
// NOLINTBEGIN(google-explicit-constructor,google-readability-casting)
21+
class B { B(int i); };
22+
// NOLINTEND(google-explicit-constructor)
23+
auto Num2 = (unsigned int)(-1);
24+
// NOLINTEND(google-readability-casting)
25+
26+
// Note: the expected output has been split over several lines so that clang-tidy
27+
// does not see the "no lint" suppression comment and mistakenly assume it
28+
// is meant for itself.
29+
// CHECK: :[[@LINE-9]]:4: error: unmatched 'NOLIN
30+
// CHECK: TBEGIN' comment without a subsequent 'NOLIN
31+
// CHECK: TEND' comment [clang-tidy-nolint]
32+
// CHECK: :[[@LINE-11]]:11: warning: single-argument constructors must be marked explicit
33+
// CHECK: :[[@LINE-11]]:4: error: unmatched 'NOLIN
34+
// CHECK: TEND' comment without a previous 'NOLIN
35+
// CHECK: TBEGIN' comment [clang-tidy-nolint]
36+
// CHECK: :[[@LINE-13]]:13: warning: C-style casts are discouraged; use static_cast
37+
// CHECK: :[[@LINE-13]]:4: error: unmatched 'NOLIN
38+
// CHECK: TEND' comment without a previous 'NOLIN
39+
// CHECK: TBEGIN' comment [clang-tidy-nolint]

0 commit comments

Comments
 (0)