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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
