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

Nathan Ridge via Phabricator via cfe-commits cfe-commits at lists.llvm.org
Sun Apr 28 15:07:23 PDT 2019

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.

  rG LLVM Github Monorepo



More information about the cfe-commits mailing list