[PATCH] D79992: [clangd] Patch PP directives to use stale preambles while building ASTs

[Sam McCall via Phabricator via cfe-commits]

sammccall accepted this revision.
sammccall added inline comments.
This revision is now accepted and ready to land.


================
Comment at: clang-tools-extra/clangd/Preamble.cpp:110
 
-/// Gets the includes in the preamble section of the file by running
-/// preprocessor over \p Contents. Returned includes do not contain resolved
-/// paths. \p VFS and \p Cmd is used to build the compiler invocation, which
-/// might stat/read files.
-llvm::Expected<std::vector<Inclusion>>
-scanPreambleIncludes(llvm::StringRef Contents,
-                     llvm::IntrusiveRefCntPtr<llvm::vfs::FileSystem> VFS,
-                     const tooling::CompileCommand &Cmd) {
+struct TextualPPDirective {
+  unsigned DirectiveLine;
----------------
this is worth a comment saying what this is used for: directives *other* than includes where we need only basic structure.


================
Comment at: clang-tools-extra/clangd/unittests/PreambleTests.cpp:205
+
+TEST(PreamblePatchTest, Define) {
+  // BAR should be defined while parsing the AST.
----------------
kadircet wrote:
> sammccall wrote:
> > do you think it makes sense to have a test that just asserts on the contents of the preamble patch? it seems like a more direct way to test some of these things.
> > 
> > These tests are nice, but debugging them seems like it might be a bit of work.
> I had 2 reasons for not doing that:
> - Not clobbering the API of `PreamblePatch` just for testing.
> - Making tests less fragile for changes in the patch format.
> 
> I suppose the first one is not that important, but I believe the latter is quite useful. We can always to something like hassubstr I suppose, but in the end we only care about its effect while creating an AST. We can do something like hassubstr, but we would still need these tests to ensure they interact as expected with the preprocessor.
Agree that having robust tests of the whole thing is important.

Maybe we could add an additional (sorry) single smoke test that asserts on the whole patch?
It'd be brittle indeed but those diffs would actually be really interesting to me as a reader of the code and reviewer of changes... gives a different perspective on what the code is doing.

I don't think having a `text()` or `textForTests()` accessor is too bad, especially if it's trivial.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D79992/new/

https://reviews.llvm.org/D79992