This is an archive of the discontinued LLVM Phabricator instance.

[clangd] Remove codeComplete that returns std::future<>
ClosedPublic

Authored by ilya-biryukov on Feb 8 2018, 4:45 AM.

Event Timeline

ilya-biryukov created this revision.Feb 8 2018, 4:45 AM
ioeric added inline comments.Feb 8 2018, 6:48 AM
unittests/clangd/SyncAPI.h
8

Any reason not to put sync APIs in ClangdServer by their async versions, so that they are easier to find? I might be missing context. But if there is a good reason, please explain in the file doc.

ilya-biryukov added inline comments.Feb 8 2018, 7:36 AM
unittests/clangd/SyncAPI.h
8

That's a good question. I don't think we want anyone using the sync versions of the API. The tests are somewhat special, though, as they'll get really complicated if we move everything to callbacks and we don't want that.

This probably deserves a comment.

ioeric added inline comments.Feb 8 2018, 9:26 AM
unittests/clangd/SyncAPI.h
8

If these are expected to be used by tests only, I'd suggest moving the file to the test directory.

ilya-biryukov added inline comments.Feb 8 2018, 10:07 AM
unittests/clangd/SyncAPI.h
8

I think it's in the unittest directory already, so we're covered here :-)

ioeric added inline comments.Feb 8 2018, 10:10 AM
unittests/clangd/SyncAPI.h
8

Sorry! My bad!

sammccall accepted this revision.Feb 8 2018, 4:25 PM

One down!

I'd like to know what you think about a generic "block the call and capture the result" mechanism rather than method-specific wrappers.
But if you're not convinced or just want to land this, I don't want to block the review.

unittests/clangd/SyncAPI.h
1

Being able to call synchronously is really nice for tests.
It's a bit unfortunate that to do this for each function we want to call synchronously (giving them a name, writing some boilerplate that repeats the args a few times).

It would be nice if we had a primitive we could compose.

Here's an idea that might work:

Tagged<CompletionList> CompletionResults;
Server.codeComplete(File, Pos, Opts, capture(CompletionResults), OverriddenContents);

capture() returns a callback that writes into CompletionResults. It also magically causes the call to block!

(brief pause for you to consider the API before I suggest a shameful implementation)

Actually capture() returns a proxy object that converts to UniqueFunction. The proxy object itself has a destructor that blocks on the UF being invoked. The proxy will be destroyed at the end of the full-expression, i.e. the line. e.g.

template <typename T> struct CaptureProxy {
  T &Target;
  std::promise<T> Promise;
  std::future<T> Future;
  CaptureProxy(T &Target) : Target(Target) {}
  operator UniqueFunction<void(T)>() {
    return BindWithForward([](T Value, std::promise<T> Promise) {
      Promise.set_value(std::move(Value));
    }, std::move(Promise));
  }
  ~CaptureProxy() { Target = Future.get(); }
};
template <typename T> CaptureProxy<T> capture(T &Target) {
  return CaptureProxy<T>(Target);
}

This is a little bit magic, but it's just for tests :-)

(One caveat - this needs the return value to be default-constructible. We could easily use Optional instead, but these are default-constructible in practice)

8

Yep, can you add a file comment describing why these exist?

This revision is now accepted and ready to land.Feb 8 2018, 4:25 PM
ilya-biryukov marked 2 inline comments as done.
  • Added CaptureProxy<T> helper, use it to implement runCodeComplete.
  • Documented that we deliberately don't expose the sync API in tests.
  • Rebased onto the latest head.
This revision was automatically updated to reflect the committed changes.
ilya-biryukov added inline comments.Feb 12 2018, 3:40 AM
unittests/clangd/SyncAPI.h
1

As discussed offline, I've opted for both:

  • implementing the helper to allow reusing the future/promise code
  • providing a function that actually returns the value synchronously.

This approach writing a small amount of wrapper code for each of the LSP methods in ClangdServer, but it, arguably, makes the test code nicer.