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

[Sam McCall via Phabricator via cfe-commits]

sammccall added inline comments.


================
Comment at: clangd/ClangdUnit.cpp:159
+
+      PP.getPPCallbacks()->InclusionDirective(
+          HashTok.getLocation(), IncludeTok, WrittenFilename, Angled,
----------------
ilya-biryukov wrote:
> This should be `Delegate->` instead of `PP.getCallbacks()`.
> Maybe add a test that catches it?
So after permuting the code, the existing test catches this :-/
Not sure why it didn't before.

It's hard to construct a test that catches this explicitly, the most obvious way would be to add a PP callbacks inside/outside the replay and ensure it does/doesn't capture the events, but there's deliberately no public API for that.

The offline discussion about using the recorded IncludeStructure seems like the best we can do through the public API, but it's pretty indirect.


================
Comment at: clangd/ClangdUnit.cpp:279
+    Clang->getPreprocessor().addPPCallbacks(llvm::make_unique<ReplayPreamble>(
+        Preamble->Includes, Clang->getPreprocessor().getPPCallbacks(),
+        Clang->getSourceManager(), Clang->getPreprocessor(),
----------------
ilya-biryukov wrote:
> 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.
To do this without further cluttering this bloated function, I wrapped this in ReplayPreamble::attach. I'm not sure I like it, but I'm out of ideas, feel free to bikeshed.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D54694