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

[Sam McCall via Phabricator via cfe-commits]

sammccall 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
----------------
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.


================
Comment at: clang-tools-extra/clangd/Preamble.cpp:431
+    for (const auto &TD : ModifiedScan->TextualDirectives)
+      Patch << TD.Text << '\n';
+  }
----------------
don't you need a #line directive too? Seems like you're not using the DirectiveLine anywhere.


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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79992