[PATCH] D38583: [clangd] Added async API to run code completion.

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added inline comments.


================
Comment at: clangd/ClangdServer.cpp:222
+
+  std::shared_ptr<PreambleData const> Preamble =
+      Resources->getPossiblyStalePreamble();
----------------
klimek wrote:
> I think we use "const type" everywhere.
Sorry, my previously preferred style keeps sneaking in.


================
Comment at: clangd/ClangdServer.cpp:225
+  // A task that will be run asynchronously.
+  PackagedTask Task([=]() mutable { // 'mutable' to reassign Preamble variable.
+    if (!Preamble) {
----------------
klimek wrote:
> It doesn't seem like we use Preamble anywhere else but in the lambda - so why not get it in the lambda?
TLDR; To use in async requests exactly the same preamble that was previously used for sync requests.

Those are two different time points. Our callers might change the file when we'll be executing this lambda. I assume that most of time we'll want the preamble that was built at the point where `codeComplete` is called, not after that.

On the other hand, after file was changed, code completion results will probably be irrelevant and will be discarded by clients anyway, so that might not matter. I've opted for not changing the behavior and using the same preamble that was previously used by the synchronous version. (Unless `Preamble` was null in the sync case, where we would only improve performance).


https://reviews.llvm.org/D38583