[PATCH] D75286: [clangd] Handle clang-tidy suppression comments for diagnostics inside macro expansions

[Sam McCall via Phabricator via cfe-commits]

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