[PATCH] D111208: [clang-tidy] Support globbing in NOLINT* expressions

[Salman Javed via Phabricator via llvm-commits]

salman-javed-nz added a comment.

In D111208#3053660 <https://reviews.llvm.org/D111208#3053660>, @carlosgalvezp wrote:

> Now, the requirements for a match are stricter (and simpler), making the code easier to reason about: any NOLINTBEGIN(X) must be matched by an identical NOLINTEND(X), for any X.

Thanks! Simplifying it right down looks to me like the right thing to do.

In case you find it hard to follow my suggested changes in Phabricator's interface, I have copied these suggestions into this attachment for convenience of reading: F19524409: changes.cpp <https://reviews.llvm.org/F19524409>



================
Comment at: clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp:325
                           StringRef Line, size_t *FoundNolintIndex = nullptr,
-                          bool *SuppressionIsSpecific = nullptr) {
+                          StringRef *FoundNolintBracket = nullptr) {
   if (FoundNolintIndex)
----------------
This feels like a more intuitive argument name to me, plus it aligns with the local variable `ChecksStr` below. What do you think?


================
Comment at: clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp:343-357
   if (BracketIndex < Line.size() && Line[BracketIndex] == '(') {
+    const size_t BracketStartIndex = BracketIndex;
     ++BracketIndex;
     const size_t BracketEndIndex = Line.find(')', BracketIndex);
     if (BracketEndIndex != StringRef::npos) {
+      if (FoundNolintBracket)
+        *FoundNolintBracket = Line.substr(
----------------
I think this should work the same without the additional index variable and substring.


================
Comment at: clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp:344
   if (BracketIndex < Line.size() && Line[BracketIndex] == '(') {
+    const size_t BracketStartIndex = BracketIndex;
     ++BracketIndex;
----------------
This is just a copy of `BracketIndex`. Would it be better to consolidate the two? 


================
Comment at: clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp:348-353
+      if (FoundNolintBracket)
+        *FoundNolintBracket = Line.substr(
+            BracketStartIndex, BracketEndIndex - BracketStartIndex + 1);
+
       StringRef ChecksStr =
           Line.substr(BracketIndex, BracketEndIndex - BracketIndex);
----------------
It gets a bit hard to follow with `BracketIndex` and `BracketStartIndex` interspersed on these lines, and a `+ 1` in one substring but not in the other substring.
Also aren't both substrings pretty much the same thing?


================
Comment at: clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp:397
   size_t NolintIndex;
-  bool SuppressionIsSpecific;
-  auto List = [&]() -> SmallVector<SourceLocation> * {
-    return SuppressionIsSpecific ? &SpecificNolintBegins : &GlobalNolintBegins;
-  };
+  StringRef NolintBracket;
   for (const auto &Line : Lines) {
----------------
To align with the name `ChecksStr` used in the other function(s).


================
Comment at: clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp:407-409
+      if (!NolintBegins.empty() &&
+          (NolintBegins.back().second == NolintBracket)) {
+        NolintBegins.pop_back();
----------------
It's not necessarily the last element of `NolintBegins` that you need to `pop_back()`.

In the case where you have overlapping `NOLINTBEGIN` regions...
```
// NOLINTBEGIN(A)  <-- push A
// NOLINTBEGIN(B) <-- push B
// NOLINTEND(A) <-- pop A
// NOLINTEND(B) <-- pop B
```

Therefore you need to search through all of the elements, not just the last one.


================
Comment at: clang-tools-extra/test/clang-tidy/infrastructure/nolintbeginend.cpp:57
 // NOLINTBEGIN(some-other-check)
-class C7 { C7(int i); };
+class C8 { C8(int i); };
 // NOLINTEND(some-other-check)
----------------
Since `C6` & `C7` have been removed you could renumber `C8` to `C6`. Once this feature is in master I don't see this test file changing again for a long time. 


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

https://reviews.llvm.org/D111208