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

[Carlos Galvez via Phabricator via llvm-commits]

carlosgalvezp added inline comments.


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


================
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);
----------------
salman-javed-nz wrote:
> 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?
I wanted "FoundCheckStr" to contain also the brackets, so that NOLINT( is different than NOLINT(). However I realize now that they are anyway treated differently (as per previous discussion) so I'll remove this and keep it simple.


================
Comment at: clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp:407-409
+      if (!NolintBegins.empty() &&
+          (NolintBegins.back().second == NolintBracket)) {
+        NolintBegins.pop_back();
----------------
salman-javed-nz wrote:
> 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.
Thanks, that's right. However that's a problem that already exists on master, right? It's pushing from the back anyway? We should probably have a unit test for this.

Would it make sense to leave that for a separate patch? Seems like the scope is growing and I'd like to keep the change as small as possible.


================
Comment at: clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp:446
     SuppressionErrors.emplace_back(Error.getValue());
-    return false;
   }
----------------
salman-javed-nz wrote:
> carlosgalvezp wrote:
> > I had to remove these "return false", otherwise I would not get errors regarding "Found NOLINTBEGIN that was unmatched by a NOLINTEND". Not really sure why they were there in the first place.
> > 
> > All unit tests pass with this patch.
> The reason for the `return false` was because the file is already determined to have a unmatched `NOLINTBEGIN`, so it's FUBAR and probably not worth checking the remainder of anyway. I don't mind either way. 
I see. In the test case that I added, I got prints that `NOLINTEND` is unmatched by a `BEGIN`, but I didn't get a message that `NOLINTBEGIN` is unmatched by an `END`, which I did get in a separate unit test, so I thought that's intended behavior. 


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

https://reviews.llvm.org/D111208