salman-javed-nz added a comment.
Thanks for taking a look. I will update the diff to address your comments. Have
a good new years break.
================
Comment at: clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp:197
+ SourceLocation Loc = FileStartLoc.getLocWithOffset(
+ static_cast<SourceLocation::IntTy>(Error.Message.FileOffset));
return diag(Error.DiagnosticName, Loc, Error.Message.Message,
----------------
kadircet wrote:
> this change looks unrelated to the current patch.
I suppose it is. It's fixing a lint issue in a function used by the NOLINTBEGIN
functionality, but not needed to implement the caching functionality. I don't
have strong feelings about whether this change is bundled in this patch or not.
================
Comment at: clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp:602
EnableNolintBlocks)) {
- ++Context.Stats.ErrorsIgnoredNOLINT;
+ // Even though this diagnostic is suppressed, there may be other issues
with
+ // the file that need raising, such as unclosed NOLINT(BEGIN/END) blocks.
----------------
kadircet wrote:
> looks like an unrelated change, maybe move to a separate patch?
This is needed for this patch. The diagnostic in question may be suppressed
(`shouldSuppressDiagnostic() = true`) but the cache generation could return
errors to do with badly formed NOLINTBEGIN blocks for other checks in the file.
================
Comment at: clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.h:76
+ // NOTE: at first glance, *it may seem* that this bool is redundant when the
+ // `Checks.empty()` method already exists.
+ // However, `Checks.empty()` cannot differentiate "NOLINT" from "NOLINT()".
----------------
kadircet wrote:
> similar to above is `NOLINT()` useful at all? maybe we should just not
> generate such tokens?
`NOLINT()` is a pretty silly statement, but it is specially mentioned in the
clang-tidy documentation.
I didn't want the parser to just swallow `NOLINT()`s as if they were never
there, because I still want to pick up errors like:
```
// NOLINTBEGIN()
// NOLINTEND(some-check)
^
Error: NOLINTEND does not match previous NOLINTBEGIN
```
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D116085/new/
https://reviews.llvm.org/D116085
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
