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

Salman Javed via Phabricator via cfe-commits cfe-commits at lists.llvm.org
Sun Oct 10 11:25:43 PDT 2021


salman-javed-nz added inline comments.


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

Actually yes, you're right. Disregard my previous comment. I was thinking of a scenario that isn't a problem.
It's all looking good to me now. 


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

https://reviews.llvm.org/D111208



More information about the cfe-commits mailing list