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