sammccall added inline comments.
================ Comment at: clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp:353 + SM.getFileID(Loc) != FileFilter) { + return false; + } ---------------- why are we breaking the loop if we hit another file ID, rather than just not checking it? I think a macro token can be expanded into another macro (with a non-main file ID) which in turn expands into the main-file - we'd want to reach the main file in that case. ================ Comment at: clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.h:215 /// to this function. -/// The `CheckMacroExpansion` parameter determines whether the function should -/// handle the case where the diagnostic is inside a macro expansion. A degree -/// of control over this is needed because handling this case can require -/// examining source files other than the one in which the diagnostic is -/// located, and in some use cases we cannot rely on such other files being -/// mapped in the SourceMapper. +/// If a valid FileID is passed as `FileFilter`, the function does not attempt +/// to read source files other than the one identified by the filter. A degree ---------------- I do wonder a how hard it would be to approach this more directly: (conditionally )use some API for pulling code out of the SourceManager that *won't* silently do IO. ContentCache::getRawBuffer() or something? Up to you whether to look at this at all of course, this approach is better than what we have now. ================ Comment at: clang-tools-extra/clangd/ParsedAST.cpp:288 // source buffer for preamble files. For the same reason, we ask // shouldSuppressDiagnostic not to follow macro expansions, since // those might take us into a preamble file as well. ---------------- this part of the comment seems stale ================ Comment at: clang-tools-extra/clangd/unittests/DiagnosticsTests.cpp:278 + #define BAD 8 / i + double f = BAD; // NOLINT + double g = [[8]] / i; ---------------- you may want a case with two levels of expansion Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D75286/new/ https://reviews.llvm.org/D75286 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits