[PATCH] D38629: [clangd] Added a callback-based codeComplete in clangd.

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added inline comments.


================
Comment at: clangd/ClangdServer.cpp:51
 
+template <class Ret, class... Args>
+std::future<Ret> makeFutureAPIFromCallback(
----------------
sammccall wrote:
> I'm not sure we need a template here rather than just writing the code directly.
Removed the template, inlined its usage.


================
Comment at: clangd/ClangdServer.cpp:257
+  // A task that will be run asynchronously.
+  auto Task =
+      // 'mutable' to reassign Preamble variable.
----------------
sammccall wrote:
> Isn't this exactly where you'd want to use ForwardBinder?
> (Yeah, it looks like the work scheduler itself has this functionality too)
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.


================
Comment at: clangd/ClangdServer.h:261
+  void codeComplete(
+      UniqueFunction<void(Tagged<std::vector<CompletionItem>>)> Callback,
+      PathRef File, Position Pos,
----------------
sammccall wrote:
> Hmm, generally having a callback as the last parameter is nicer, but this doesn't play nicely with optional params...
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).


https://reviews.llvm.org/D38629