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