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

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet marked 3 inline comments as done.
kadircet added inline comments.


================
Comment at: clang-tools-extra/clangd/Preamble.cpp:419
+  if (DirectivesChanged) {
+    // We need to patch all the directives, since they are order dependent. e.g:
+    // #define BAR(X) NEW(X) // Newly introduced in Modified
----------------
sammccall wrote:
> Hmm, with N macros in the file and O(1) changed, it seems like the inaccuracy of the conditional-scanning may outweigh defining that one in the wrong order in complex cases.
> 
> Especially since this only seems to come up if there are multiple definitions of the same macro, which seems easy enough to detect if that's an important case.
makes sense, will change it to only patch `new` defines. And actually most of the time any define directive below this one would also be considered new, because line information has changed for them, hence we need to patch. Unless user both inserts and deletes a line.


================
Comment at: clang-tools-extra/clangd/Preamble.cpp:431
+    for (const auto &TD : ModifiedScan->TextualDirectives)
+      Patch << TD.Text << '\n';
+  }
----------------
sammccall wrote:
> don't you need a #line directive too? Seems like you're not using the DirectiveLine anywhere.
spoileers. that's in D80198, I didn't want to make this one any bigger.


================
Comment at: clang-tools-extra/clangd/unittests/PreambleTests.cpp:205
+
+TEST(PreamblePatchTest, Define) {
+  // BAR should be defined while parsing the AST.
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79992