ilya-biryukov added inline comments.
================
Comment at: clangd/TUScheduler.cpp:357
+
+ bool CanReuseAST = InputsAreTheSame && OldPreamble == NewPreamble;
{
----------------
ioeric wrote:
> nit: `(OldPreamble == NewPreamble)`
>
> Do you intend to compare the shared pointer instead of the values?
Yes, this is intentional. `buildPreamble` will either return the old one, if it
can be reused, or build the new one.
================
Comment at: clangd/TUScheduler.cpp:360
std::lock_guard<std::mutex> Lock(Mutex);
+ OldPreamble.reset();
if (NewPreamble)
----------------
ioeric wrote:
> Why reset?
We don't need the old preamble at this point, so we give it a chance to die (if
there are no more references).
Note that there's an expensive operation that follows (building the AST), so
removing the preamble before it seems like a win
================
Comment at: clangd/TUScheduler.cpp:373
+ // FIXME(ibiryukov): the AST could actually change if non-preamble
+ // includes changed, but we choose to ignore it.
+ // FIXME(ibiryukov): should we refresh the cache in IdleASTs for the
----------------
ioeric wrote:
> Do we choose to ignore this because it's rare? What are non-preamble
> #includes?
Exactly, we don't have the code to track and check for all accessed files.
This should be rare, hopefully won't show up in practice.
================
Comment at: test/clangd/extra-flags.test:26
---
-{"jsonrpc":"2.0","method":"textDocument/didChange","params":{"textDocument":{"uri":"test:///foo.c","version":2},"contentChanges":[{"text":"int
main() { int i; return i; }"}]}}
+{"jsonrpc":"2.0","method":"textDocument/didChange","params":{"textDocument":{"uri":"test:///foo.c","version":2},"contentChanges":[{"text":"int
main() { int i; return i+1; }"}]}}
# CHECK: "method": "textDocument/publishDiagnostics",
----------------
ioeric wrote:
> Why this change?
The test expects two publishDiagnostics, but it won't get the second one since
if the contents of the file are the same :-)
So I altered the contents a bit to make sure we get the second result too
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D49783
_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
