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


cfe-commits mailing list

Reply via email to