[PATCH] D60953: [clangd] Respect clang-tidy suppression comments

[Nathan Ridge via Phabricator via cfe-commits]

nridge added inline comments.


================
Comment at: clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp:395
+         DiagLevel != DiagnosticsEngine::Fatal &&
+         LineIsMarkedWithNOLINTinMacro(Info.getSourceManager(),
+                                       Info.getLocation(), Info.getID(),
----------------
sammccall wrote:
> 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.
Thanks for pointing this out! This should be addressed in the updated patch.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D60953