This is an archive of the discontinued LLVM Phabricator instance.

[clangd] Added async API to run code completion.
ClosedPublic

Authored by ilya-biryukov on Oct 5 2017, 6:19 AM.

Details

Summary

ClangdServer now provides async code completion API.
It is still used synchronously by ClangdLSPServer, more work is needed
to allow processing other requests in parallel while completion (or
any other request) is running.

Diff Detail

Repository
rL LLVM

Event Timeline

ilya-biryukov created this revision.Oct 5 2017, 6:19 AM
klimek added inline comments.Oct 5 2017, 6:32 AM
clangd/ClangdServer.cpp
222 ↗(On Diff #117807)

I think we use "const type" everywhere.

225 ↗(On Diff #117807)

It doesn't seem like we use Preamble anywhere else but in the lambda - so why not get it in the lambda?

  • Moved 'const' to the left.
ilya-biryukov added inline comments.Oct 5 2017, 6:46 AM
clangd/ClangdServer.cpp
222 ↗(On Diff #117807)

Sorry, my previously preferred style keeps sneaking in.

225 ↗(On Diff #117807)

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

klimek added inline comments.Oct 5 2017, 6:51 AM
clangd/ClangdServer.cpp
225 ↗(On Diff #117807)

That makes sense, but needs a comment in the code :)

clangd/ClangdServer.h
236–237 ↗(On Diff #117811)

Extra space before asynchronously.
Replace first ',' with ';'
... wait for the results of the async request...

  • Added a comment about Preamble assignment.
ilya-biryukov marked an inline comment as done.
  • Fixed grammar in a comment.
ilya-biryukov added inline comments.Oct 5 2017, 7:57 AM
clangd/ClangdServer.h
236–237 ↗(On Diff #117811)

Split into two sentences on , instead of replacing it with ;. Feels even better.

ilya-biryukov marked 2 inline comments as done.Oct 5 2017, 7:58 AM
klimek accepted this revision.Oct 5 2017, 8:11 AM

LG

clangd/ClangdServer.h
236–237 ↗(On Diff #117811)

..."the" returned future...

This revision is now accepted and ready to land.Oct 5 2017, 8:11 AM
  • Added another missing 'the'.
ilya-biryukov marked an inline comment as done.Oct 5 2017, 10:02 AM
This revision was automatically updated to reflect the committed changes.