Page MenuHomePhabricator

[clangd] Added move-only function helpers.
ClosedPublic

Authored by ilya-biryukov on Oct 6 2017, 7:47 AM.

Details

Summary

They are now used in ClangdScheduler instead of deferred std::async
computations.
The results of std::async are much less effective and do not provide
a good abstraction for similar purposes, i.e. for storing additional callbacks
to clangd async tasks. The actual callback API will follow a bit later.

Diff Detail

Repository
rL LLVM

Event Timeline

ilya-biryukov created this revision.Oct 6 2017, 7:47 AM
  • Fix to ForwardBinder.
  • Add UniqueFunction(nullptr) constructor.
  • Added missing STL headers to Function.h
ilya-biryukov retitled this revision from [clangd] Added a move-only function helpers. to [clangd] Added move-only function helpers..Oct 6 2017, 8:54 AM
  • Use proper types (Args&&) when forwarding arguments.
sammccall accepted this revision.Oct 9 2017, 1:35 AM
sammccall added inline comments.
clangd/Function.h
9 ↗(On Diff #118184)

Maybe add a file comment "provides analogues to std::function that supports move semantics"?

36 ↗(On Diff #118184)

Do you want this constructor to be explicit?

If not, I think you should be able to simplify the callsites in ClangdServer.h

77 ↗(On Diff #118184)

nit: just 'moved'? std::move is just a cast...

117 ↗(On Diff #118184)

I find these "first arg" APIs a bit awkward, and can lead to writing confusing APIs for easier binding. Not sure there's a good alternative, though.

121 ↗(On Diff #118184)

nit: can -> must?

This revision is now accepted and ready to land.Oct 9 2017, 1:35 AM
ilya-biryukov marked 3 inline comments as done.

Addressed review comments.

  • Added a file comment.
  • Simplified callsites of UniqueFunction in ClangdServer.h
  • Properly forward UniqueFunction's constructor argument.
  • Updates to comments.
clangd/Function.h
36 ↗(On Diff #118184)

Thanks for spotting this.
I think implicit works fine here. std::function also has implicit constructors. Simplified the callsites.

117 ↗(On Diff #118184)

Yeah. Works just like std::bind, though. And I would be very happy to remove that particular function altogether when we'll be able to use lambda initializer in LLVM codebase.

This revision was automatically updated to reflect the committed changes.

Excuse me, Ubuntu 14.04's g++-4.8.4 doesn't compile it. :(
Any idea?

In file included from clang-tools-extra/clangd/ClangdServer.h:23:0,
                 from clang-tools-extra/clangd/ClangdServer.cpp:10:
clang-tools-extra/clangd/Function.h: In instantiation of ‘Ret clang::clangd::UniqueFunction<Ret(Args ...)>::FunctionCallImpl<Callable>::Call(Args ...) [with Callable = clang::clangd::ForwardBinder<clang::clangd::ClangdServer::scheduleCancelRebuild(std::shared_ptr<clang::clangd::CppFile>)::__lambda23, std::promise<void>, std::future<void> >; Ret = void; Args = {}]’:
clang-tools-extra/clangd/ClangdServer.cpp:484:1:   required from here
clang-tools-extra/clangd/Function.h:68:74: error: no match for call to ‘(clang::clangd::ForwardBinder<clang::clangd::ClangdServer::scheduleCancelRebuild(std::shared_ptr<clang::clangd::CppFile>)::__lambda23, std::promise<void>, std::future<void> >) ()’
     Ret Call(Args... As) override { return Func(std::forward<Args>(As)...); }
                                                                          ^
clang-tools-extra/clangd/Function.h:82:45: note: candidate is:
 template <class Func, class... Args> struct ForwardBinder {
                                             ^
clang-tools-extra/clangd/Function.h:108:8: note: template<class ... RestArgs> decltype (CallImpl(llvm::index_sequence_for<Iters ...>(), (forward<RestArgs>)(clang::clangd::ForwardBinder::operator()::Rest)...)) clang::clangd::ForwardBinder<Func, Args>::operator()(RestArgs&& ...) [with RestArgs = {RestArgs ...}; Func = clang::clangd::ClangdServer::scheduleCancelRebuild(std::shared_ptr<clang::clangd::CppFile>)::__lambda23; Args = {std::promise<void>, std::future<void>}]
   auto operator()(RestArgs &&... Rest)
        ^
clang-tools-extra/clangd/Function.h:108:8: note:   template argument deduction/substitution failed:
clang-tools-extra/clangd/Function.h: In substitution of ‘template<class ... RestArgs> decltype (CallImpl(llvm::index_sequence_for<Iters ...>(), (forward<RestArgs>)(clang::clangd::ForwardBinder::operator()::Rest)...)) clang::clangd::ForwardBinder<Func, Args>::operator()(RestArgs&& ...) [with RestArgs = {RestArgs ...}; Func = clang::clangd::ClangdServer::scheduleCancelRebuild(std::shared_ptr<clang::clangd::CppFile>)::__lambda23; Args = {std::promise<void>, std::future<void>}] [with RestArgs = {}]’:
clang-tools-extra/clangd/Function.h:68:74:   required from ‘Ret clang::clangd::UniqueFunction<Ret(Args ...)>::FunctionCallImpl<Callable>::Call(Args ...) [with Callable = clang::clangd::ForwardBinder<clang::clangd::ClangdServer::scheduleCancelRebuild(std::shared_ptr<clang::clangd::CppFile>)::__lambda23, std::promise<void>, std::future<void> >; Ret = void; Args = {}]’
clang-tools-extra/clangd/ClangdServer.cpp:484:1:   required from here
clang-tools-extra/clangd/Function.h:110:59: error: cannot call member function ‘decltype (get<0>(this->.FuncWithArguments)((forward<Args>)((get<Indexes + 1>)(this->.FuncWithArguments))..., (forward<RestArgs>)(clang::clangd::ForwardBinder::CallImpl::Rest)...)) clang::clangd::ForwardBinder<Func, Args>::CallImpl(llvm::integer_sequence<long unsigned int, Indexes ...>, RestArgs&& ...) [with long unsigned int ...Indexes = {0ul, 1ul}; RestArgs = {}; Func = clang::clangd::ClangdServer::scheduleCancelRebuild(std::shared_ptr<clang::clangd::CppFile>)::__lambda23; Args = {std::promise<void>, std::future<void>}; decltype (get<0>(this->.FuncWithArguments)((forward<Args>)((get<Indexes + 1>)(this->.FuncWithArguments))..., (forward<RestArgs>)(clang::clangd::ForwardBinder::CallImpl::Rest)...)) = void]’ without object
                            std::forward<RestArgs>(Rest)...)) {
                                                           ^
clang-tools-extra/clangd/Function.h: In instantiation of ‘Ret clang::clangd::UniqueFunction<Ret(Args ...)>::FunctionCallImpl<Callable>::Call(Args ...) [with Callable = clang::clangd::ForwardBinder<clang::clangd::ClangdServer::scheduleCancelRebuild(std::shared_ptr<clang::clangd::CppFile>)::__lambda23, std::promise<void>, std::future<void> >; Ret = void; Args = {}]’:
clang-tools-extra/clangd/ClangdServer.cpp:484:1:   required from here
clang-tools-extra/clangd/Function.h:68:74: error: return-statement with a value, in function returning 'void' [-fpermissive]
     Ret Call(Args... As) override { return Func(std::forward<Args>(As)...); }
                                                                          ^
clang-tools-extra/clangd/Function.h: In instantiation of ‘Ret clang::clangd::UniqueFunction<Ret(Args ...)>::FunctionCallImpl<Callable>::Call(Args ...) [with Callable = clang::clangd::ForwardBinder<clang::clangd::ClangdServer::scheduleReparseAndDiags(clang::clangd::PathRef, clang::clangd::VersionedDraft, std::shared_ptr<clang::clangd::CppFile>, clang::clangd::Tagged<llvm::IntrusiveRefCntPtr<clang::vfs::FileSystem> >)::__lambda22, std::future<llvm::Optional<std::vector<clang::clangd::DiagWithFixIts, std::allocator<clang::clangd::DiagWithFixIts> > > >, std::promise<void> >; Ret = void; Args = {}]’:
clang-tools-extra/clangd/ClangdServer.cpp:484:1:   required from here
clang-tools-extra/clangd/Function.h:68:74: error: no match for call to ‘(clang::clangd::ForwardBinder<clang::clangd::ClangdServer::scheduleReparseAndDiags(clang::clangd::PathRef, clang::clangd::VersionedDraft, std::shared_ptr<clang::clangd::CppFile>, clang::clangd::Tagged<llvm::IntrusiveRefCntPtr<clang::vfs::FileSystem> >)::__lambda22, std::future<llvm::Optional<std::vector<clang::clangd::DiagWithFixIts, std::allocator<clang::clangd::DiagWithFixIts> > > >, std::promise<void> >) ()’
clang-tools-extra/clangd/Function.h:82:45: note: candidate is:
 template <class Func, class... Args> struct ForwardBinder {
                                             ^
clang-tools-extra/clangd/Function.h:108:8: note: template<class ... RestArgs> decltype (CallImpl(llvm::index_sequence_for<Iters ...>(), (forward<RestArgs>)(clang::clangd::ForwardBinder::operator()::Rest)...)) clang::clangd::ForwardBinder<Func, Args>::operator()(RestArgs&& ...) [with RestArgs = {RestArgs ...}; Func = clang::clangd::ClangdServer::scheduleReparseAndDiags(clang::clangd::PathRef, clang::clangd::VersionedDraft, std::shared_ptr<clang::clangd::CppFile>, clang::clangd::Tagged<llvm::IntrusiveRefCntPtr<clang::vfs::FileSystem> >)::__lambda22; Args = {std::future<llvm::Optional<std::vector<clang::clangd::DiagWithFixIts, std::allocator<clang::clangd::DiagWithFixIts> > > >, std::promise<void>}]
   auto operator()(RestArgs &&... Rest)
        ^
clang-tools-extra/clangd/Function.h:108:8: note:   template argument deduction/substitution failed:
clang-tools-extra/clangd/Function.h: In substitution of ‘template<class ... RestArgs> decltype (CallImpl(llvm::index_sequence_for<Iters ...>(), (forward<RestArgs>)(clang::clangd::ForwardBinder::operator()::Rest)...)) clang::clangd::ForwardBinder<Func, Args>::operator()(RestArgs&& ...) [with RestArgs = {RestArgs ...}; Func = clang::clangd::ClangdServer::scheduleReparseAndDiags(clang::clangd::PathRef, clang::clangd::VersionedDraft, std::shared_ptr<clang::clangd::CppFile>, clang::clangd::Tagged<llvm::IntrusiveRefCntPtr<clang::vfs::FileSystem> >)::__lambda22; Args = {std::future<llvm::Optional<std::vector<clang::clangd::DiagWithFixIts, std::allocator<clang::clangd::DiagWithFixIts> > > >, std::promise<void>}] [with RestArgs = {}]’:
clang-tools-extra/clangd/Function.h:68:74:   required from ‘Ret clang::clangd::UniqueFunction<Ret(Args ...)>::FunctionCallImpl<Callable>::Call(Args ...) [with Callable = clang::clangd::ForwardBinder<clang::clangd::ClangdServer::scheduleReparseAndDiags(clang::clangd::PathRef, clang::clangd::VersionedDraft, std::shared_ptr<clang::clangd::CppFile>, clang::clangd::Tagged<llvm::IntrusiveRefCntPtr<clang::vfs::FileSystem> >)::__lambda22, std::future<llvm::Optional<std::vector<clang::clangd::DiagWithFixIts, std::allocator<clang::clangd::DiagWithFixIts> > > >, std::promise<void> >; Ret = void; Args = {}]’
clang-tools-extra/clangd/ClangdServer.cpp:484:1:   required from here
clang-tools-extra/clangd/Function.h:110:59: error: cannot call member function ‘decltype (get<0>(this->.FuncWithArguments)((forward<Args>)((get<Indexes + 1>)(this->.FuncWithArguments))..., (forward<RestArgs>)(clang::clangd::ForwardBinder::CallImpl::Rest)...)) clang::clangd::ForwardBinder<Func, Args>::CallImpl(llvm::integer_sequence<long unsigned int, Indexes ...>, RestArgs&& ...) [with long unsigned int ...Indexes = {0ul, 1ul}; RestArgs = {}; Func = clang::clangd::ClangdServer::scheduleReparseAndDiags(clang::clangd::PathRef, clang::clangd::VersionedDraft, std::shared_ptr<clang::clangd::CppFile>, clang::clangd::Tagged<llvm::IntrusiveRefCntPtr<clang::vfs::FileSystem> >)::__lambda22; Args = {std::future<llvm::Optional<std::vector<clang::clangd::DiagWithFixIts, std::allocator<clang::clangd::DiagWithFixIts> > > >, std::promise<void>}; decltype (get<0>(this->.FuncWithArguments)((forward<Args>)((get<Indexes + 1>)(this->.FuncWithArguments))..., (forward<RestArgs>)(clang::clangd::ForwardBinder::CallImpl::Rest)...)) = void]’ without object
                            std::forward<RestArgs>(Rest)...)) {
                                                           ^
clang-tools-extra/clangd/Function.h: In instantiation of ‘Ret clang::clangd::UniqueFunction<Ret(Args ...)>::FunctionCallImpl<Callable>::Call(Args ...) [with Callable = clang::clangd::ForwardBinder<clang::clangd::ClangdServer::scheduleReparseAndDiags(clang::clangd::PathRef, clang::clangd::VersionedDraft, std::shared_ptr<clang::clangd::CppFile>, clang::clangd::Tagged<llvm::IntrusiveRefCntPtr<clang::vfs::FileSystem> >)::__lambda22, std::future<llvm::Optional<std::vector<clang::clangd::DiagWithFixIts, std::allocator<clang::clangd::DiagWithFixIts> > > >, std::promise<void> >; Ret = void; Args = {}]’:
clang-tools-extra/clangd/ClangdServer.cpp:484:1:   required from here
clang-tools-extra/clangd/Function.h:68:74: error: return-statement with a value, in function returning 'void' [-fpermissive]
     Ret Call(Args... As) override { return Func(std::forward<Args>(As)...); }
                                                                          ^
clang-tools-extra/clangd/Function.h: In instantiation of ‘Ret clang::clangd::UniqueFunction<Ret(Args ...)>::FunctionCallImpl<Callable>::Call(Args ...) [with Callable = clang::clangd::ForwardBinder<clang::clangd::ClangdServer::codeComplete(clang::clangd::PathRef, clang::clangd::Position, llvm::Optional<llvm::StringRef>, llvm::IntrusiveRefCntPtr<clang::vfs::FileSystem>*)::__lambda17, std::packaged_task<clang::clangd::Tagged<std::vector<clang::clangd::CompletionItem, std::allocator<clang::clangd::CompletionItem> > >()> >; Ret = void; Args = {}]’:
clang-tools-extra/clangd/ClangdServer.cpp:484:1:   required from here
clang-tools-extra/clangd/Function.h:68:74: error: no match for call to ‘(clang::clangd::ForwardBinder<clang::clangd::ClangdServer::codeComplete(clang::clangd::PathRef, clang::clangd::Position, llvm::Optional<llvm::StringRef>, llvm::IntrusiveRefCntPtr<clang::vfs::FileSystem>*)::__lambda17, std::packaged_task<clang::clangd::Tagged<std::vector<clang::clangd::CompletionItem, std::allocator<clang::clangd::CompletionItem> > >()> >) ()’
clang-tools-extra/clangd/Function.h:82:45: note: candidate is:
 template <class Func, class... Args> struct ForwardBinder {
                                             ^
clang-tools-extra/clangd/Function.h:108:8: note: template<class ... RestArgs> decltype (CallImpl(llvm::index_sequence_for<Iters ...>(), (forward<RestArgs>)(clang::clangd::ForwardBinder::operator()::Rest)...)) clang::clangd::ForwardBinder<Func, Args>::operator()(RestArgs&& ...) [with RestArgs = {RestArgs ...}; Func = clang::clangd::ClangdServer::codeComplete(clang::clangd::PathRef, clang::clangd::Position, llvm::Optional<llvm::StringRef>, llvm::IntrusiveRefCntPtr<clang::vfs::FileSystem>*)::__lambda17; Args = {std::packaged_task<clang::clangd::Tagged<std::vector<clang::clangd::CompletionItem, std::allocator<clang::clangd::CompletionItem> > >()>}]
   auto operator()(RestArgs &&... Rest)
        ^
clang-tools-extra/clangd/Function.h:108:8: note:   template argument deduction/substitution failed:
clang-tools-extra/clangd/Function.h: In substitution of ‘template<class ... RestArgs> decltype (CallImpl(llvm::index_sequence_for<Iters ...>(), (forward<RestArgs>)(clang::clangd::ForwardBinder::operator()::Rest)...)) clang::clangd::ForwardBinder<Func, Args>::operator()(RestArgs&& ...) [with RestArgs = {RestArgs ...}; Func = clang::clangd::ClangdServer::codeComplete(clang::clangd::PathRef, clang::clangd::Position, llvm::Optional<llvm::StringRef>, llvm::IntrusiveRefCntPtr<clang::vfs::FileSystem>*)::__lambda17; Args = {std::packaged_task<clang::clangd::Tagged<std::vector<clang::clangd::CompletionItem, std::allocator<clang::clangd::CompletionItem> > >()>}] [with RestArgs = {}]’:
clang-tools-extra/clangd/Function.h:68:74:   required from ‘Ret clang::clangd::UniqueFunction<Ret(Args ...)>::FunctionCallImpl<Callable>::Call(Args ...) [with Callable = clang::clangd::ForwardBinder<clang::clangd::ClangdServer::codeComplete(clang::clangd::PathRef, clang::clangd::Position, llvm::Optional<llvm::StringRef>, llvm::IntrusiveRefCntPtr<clang::vfs::FileSystem>*)::__lambda17, std::packaged_task<clang::clangd::Tagged<std::vector<clang::clangd::CompletionItem, std::allocator<clang::clangd::CompletionItem> > >()> >; Ret = void; Args = {}]’
clang-tools-extra/clangd/ClangdServer.cpp:484:1:   required from here
clang-tools-extra/clangd/Function.h:110:59: error: cannot call member function ‘decltype (get<0>(this->.FuncWithArguments)((forward<Args>)((get<Indexes + 1>)(this->.FuncWithArguments))..., (forward<RestArgs>)(clang::clangd::ForwardBinder::CallImpl::Rest)...)) clang::clangd::ForwardBinder<Func, Args>::CallImpl(llvm::integer_sequence<long unsigned int, Indexes ...>, RestArgs&& ...) [with long unsigned int ...Indexes = {0ul}; RestArgs = {}; Func = clang::clangd::ClangdServer::codeComplete(clang::clangd::PathRef, clang::clangd::Position, llvm::Optional<llvm::StringRef>, llvm::IntrusiveRefCntPtr<clang::vfs::FileSystem>*)::__lambda17; Args = {std::packaged_task<clang::clangd::Tagged<std::vector<clang::clangd::CompletionItem, std::allocator<clang::clangd::CompletionItem> > >()>}; decltype (get<0>(this->.FuncWithArguments)((forward<Args>)((get<Indexes + 1>)(this->.FuncWithArguments))..., (forward<RestArgs>)(clang::clangd::ForwardBinder::CallImpl::Rest)...)) = void]’ without object
                            std::forward<RestArgs>(Rest)...)) {
                                                           ^
clang-tools-extra/clangd/Function.h: In instantiation of ‘Ret clang::clangd::UniqueFunction<Ret(Args ...)>::FunctionCallImpl<Callable>::Call(Args ...) [with Callable = clang::clangd::ForwardBinder<clang::clangd::ClangdServer::codeComplete(clang::clangd::PathRef, clang::clangd::Position, llvm::Optional<llvm::StringRef>, llvm::IntrusiveRefCntPtr<clang::vfs::FileSystem>*)::__lambda17, std::packaged_task<clang::clangd::Tagged<std::vector<clang::clangd::CompletionItem, std::allocator<clang::clangd::CompletionItem> > >()> >; Ret = void; Args = {}]’:
clang-tools-extra/clangd/ClangdServer.cpp:484:1:   required from here
clang-tools-extra/clangd/Function.h:68:74: error: return-statement with a value, in function returning 'void' [-fpermissive]
     Ret Call(Args... As) override { return Func(std::forward<Args>(As)...); }
                                                                          ^

Excuse me, Ubuntu 14.04's g++-4.8.4 doesn't compile it. :(
Any idea?

Sorry about that. Fixed in rL315284.
clang and gcc (at least version 4.8) behave differently here, so I didn't catch it before submitting.