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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
