sammccall added a comment.
Thanks for doing this!
================
Comment at: clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp:395
+ DiagLevel != DiagnosticsEngine::Fatal &&
+ LineIsMarkedWithNOLINTinMacro(Info.getSourceManager(),
+ Info.getLocation(), Info.getID(),
----------------
There may be a trap here for clangd.
When building an AST with a preamble (which is when we run clang-tidy checks),
the source code of the main file is mapped in the SourceManager as usual, but
headers are not.
An attempt to get the buffer results in the SourceManager reading the file from
disk. If the content has changed then all sorts of invariants break and clang
will typically crash.
@ilya-biryukov knows the details here better than me.
Of course this is only true for trying to read from header file locations, and
we only run clang-tidy over main-file-decls. But:
- clang-tidy checks can emit diagnostics anywhere, even outside the AST range
they run over directly
- while StoreDiags does filter out diagnostics that aren't in the main file,
this is pretty nuanced (e.g. a note in the main file is sufficient to include)
and this logic runs after the filtering added by this patch
- the way that LineIsMarkedWithNOLINTinMacro loops suggests that even if the
diagnostic is in the main-file, we'll look for NOLINT in other files
Maybe this means we can't accurately calculate suppression for clang-tidy
warnings in macro locations, or outside the main file. I think *always*
filtering these out (on the clangd side) might be OK.
================
Comment at: clang-tools-extra/clangd/ClangdUnit.cpp:254
+ tidy::ShouldSuppressDiagnostic(DiagLevel, Info, *CTContext)) {
+ // Ignored a warning, should ignore related notes as well
+ LastErrorWasIgnored = true;
----------------
the interaction between ClangdDiagnosticConsumer and StoreDiags seems a little
mysterious - I think the total behavior would be easier to understand (and
debug) if StoreDiags knew about the filtering.
At the same time, I see why you want to keep the clang-tidy-specific logic in
this file.
Maybe we could add a `filterDiagnostics(function<bool(Diagnostic&)>)` or so
into StoreDiags, similar to `contributeFixes`? That way the policy stuff (which
diagnostics to filter) stays here, and the mechanism (e.g. tracking associated
notes) is visible in StoreDiags.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D60953/new/
https://reviews.llvm.org/D60953
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
