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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
