Page MenuHomePhabricator

[clangd] Added a callback-based codeComplete in clangd.
ClosedPublic

Authored by ilya-biryukov on Oct 6 2017, 8:49 AM.

Diff Detail

Repository
rL LLVM

Event Timeline

ilya-biryukov created this revision.Oct 6 2017, 8:49 AM

I hope the whole design doesn't seem overly complicated. Very much open to suggestions on how to simplify it :-)

sammccall edited edge metadata.Oct 9 2017, 1:41 AM

I'm missing some context for this one:

  • what's the goal? Make the code read more naturally, change the async model, or something else?
  • what's the end state for codeComplete specifically? will we switch to the new overload and delete the other, or is makeFutureAPIFromCallback here to stay?
  • Added a deprecation notice to function description.

We had a discussion with @sammccall offline, probably

  • what's the goal? Make the code read more naturally, change the async model, or something else?

Callback API is more flexible (if std::future that we use had then, they'd be equivalent).
We have internal clients that want to be notified about results of code completion without

  • what's the end state for codeComplete specifically? will we switch to the new overload and delete the other, or is makeFutureAPIFromCallback here to stay?

I think the way to go would be exactly that, switch to the new API and remove the older one. There are usages in tests, ClangdLSPServer and in our internal client, but I'll remove them with a separate commit later.
Added a deprecation notice for the API.

sammccall added inline comments.Oct 13 2017, 2:27 AM
clangd/ClangdServer.cpp
51 ↗(On Diff #118225)

I'm not sure we need a template here rather than just writing the code directly.

257 ↗(On Diff #118225)

Isn't this exactly where you'd want to use ForwardBinder?
(Yeah, it looks like the work scheduler itself has this functionality too)

clangd/ClangdServer.h
261 ↗(On Diff #118225)

Hmm, generally having a callback as the last parameter is nicer, but this doesn't play nicely with optional params...

ilya-biryukov marked an inline comment as done.
  • Inlined makeFutureAPIFromCallback.
ilya-biryukov added inline comments.Oct 22 2017, 11:58 PM
clangd/ClangdServer.cpp
51 ↗(On Diff #118225)

Removed the template, inlined its usage.

257 ↗(On Diff #118225)

Yes, ClangdServer uses ForwardBinder internally.

We could refactor it to accept UniqueFunction instead, but I suggest we do that in a separate commit since there are more call sites that we'll have to change.

clangd/ClangdServer.h
261 ↗(On Diff #118225)

Right, so I opted for making it a first parameter.
I don't know if there's a better alternative. Do you think making it a last non-optional parameter would make things better? (Probably it will, at least calls without non-optional params would look nicer).

rwols added a subscriber: rwols.Oct 23 2017, 12:14 AM
sammccall accepted this revision.Oct 24 2017, 6:10 AM
This revision is now accepted and ready to land.Oct 24 2017, 6:10 AM
This revision was automatically updated to reflect the committed changes.