[PATCH] D54694: [clangd] Replay preamble #includes to clang-tidy checks.

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added inline comments.


================
Comment at: clangd/ClangdUnit.cpp:129
+class ReplayPreamble : public PPCallbacks {
+  const IncludeStructure &Includes;
+  PPCallbacks *Delegate;
----------------
Maybe move fields and the private function to the end of the class?
We definitely don't have a strict rule about this in clangd, but that's what most of the code does. The upside is that the public interface of the class is the first thing that readers of the code see.

That's a stylistic thing, so definitely feel free to disagree :-)


================
Comment at: clangd/ClangdUnit.cpp:159
+
+      PP.getPPCallbacks()->InclusionDirective(
+          HashTok.getLocation(), IncludeTok, WrittenFilename, Angled,
----------------
This should be `Delegate->` instead of `PP.getCallbacks()`.
Maybe add a test that catches it?


================
Comment at: clangd/ClangdUnit.cpp:165
+      if (File)
+        PP.getPPCallbacks()->FileSkipped(*File, FilenameTok, Inc.FileKind);
+      else {
----------------
Another one: should this be `Delegate->`?


================
Comment at: clangd/ClangdUnit.cpp:279
+    Clang->getPreprocessor().addPPCallbacks(llvm::make_unique<ReplayPreamble>(
+        Preamble->Includes, Clang->getPreprocessor().getPPCallbacks(),
+        Clang->getSourceManager(), Clang->getPreprocessor(),
----------------
Maybe assert that the pointer returned from `getPPCallbacks()` actually changes after the call to `addPPCallbacks` call here?
A potential breakage might occur if the implementation of adding a callback changes and uses an inheritor of `PPCallbacks`  that stores a list internally and adds new callbacks to a list instead of wrapping an existing one.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D54694