This is an archive of the discontinued LLVM Phabricator instance.

[clangd] Make functions of ClangdServer callback-based
ClosedPublic

Authored by ilya-biryukov on Feb 13 2018, 2:29 AM.

Details

Summary

As a consequence, all LSP operations are now handled asynchronously,
i.e. they never block the main processing thread. However, if
-run-synchronously flag is specified, clangd still runs everything on
the main thread.

Diff Detail

Repository
rL LLVM

Event Timeline

ilya-biryukov created this revision.Feb 13 2018, 2:29 AM
ioeric added inline comments.Feb 13 2018, 10:00 AM
clangd/ClangdLSPServer.cpp
194 ↗(On Diff #134014)

nit: since we are not spelling out the return type here, it might be clearer if we do:

replyError(...);
return;

return replyError(...) makes me wonder what the return type is.

clangd/ClangdServer.cpp
209 ↗(On Diff #134014)

nit: I'd probably use a different name than Callback for this parameter for clarity.

441 ↗(On Diff #134014)

Consider spelling out the captured values, just in case new variables which are not expected to be captured are added in the future.

unittests/clangd/SyncAPI.cpp
69 ↗(On Diff #134014)

I'd expect this to be checked by callers. Would return std::move(Result); work?

  • Capture only needed vars in lambda, not everything.
  • Rebase onto head.
clangd/ClangdLSPServer.cpp
194 ↗(On Diff #134014)

This trick was used in the code before (there are usages below), so I figured it's ok to do this.
If it's confusing I'll change it everywhere.

clangd/ClangdServer.cpp
209 ↗(On Diff #134014)

I actually think we should keep it this way. It's a bit tricky to grasp, but it has two important advantages:

  • Makes referencing Callback from outer scope impossible
  • saves us from coming up with names for that superficial variable, i.e. should it be called InnerCallback, Callback2, C or something else?

Is it that too confusing or do you feel we can keep it?

441 ↗(On Diff #134014)

Thanks for spotting this!

unittests/clangd/SyncAPI.cpp
69 ↗(On Diff #134014)

The reason why we need it is because capture(Result) writes return value of the callback to the Result variable. But we have to first check the default-constructed value that was there before calling Server.signatureHelp

ioeric added inline comments.Feb 13 2018, 10:57 AM
clangd/ClangdLSPServer.cpp
194 ↗(On Diff #134014)

I guess it's not confusing after figuring out what replyError returns. There is obviously tradeoff between LOC and readability. Either is not too bad. So up to you :)

clangd/ClangdServer.cpp
209 ↗(On Diff #134014)

Makes referencing Callback from outer scope impossible.

For lambdas, I think we should rely on captures for this instead of the parameter names.

saves us from coming up with names for that superficial variable, i.e. should it be called InnerCallback, Callback2, C or something else?

I thought this is a common practice with llvm coding style :P I would vote for C :)

Is it that too confusing or do you feel we can keep it?

It is a bit confusing when I first looked at it, but nothing is *too* confusing as long as the code is correct ;) I just think it would make the code easier to read.

unittests/clangd/SyncAPI.cpp
69 ↗(On Diff #134014)

I think we should probably fix the behavior of capture since this changes the behavior of llvm::Expected in user code - the check is there to make sure users always check the status. But here we always do this for users.

sammccall accepted this revision.Feb 13 2018, 11:16 AM

LG, suggest a tweak to capture() though.

I wonder whether we want to introduce using Callback<T...> = UniqueFunction<void(T...)> for readability at some point...

clangd/ClangdServer.cpp
209 ↗(On Diff #134014)

I agree with you both :-)
Conceptually, this *is* a capture - BindWithForward simulates move-capture that C++11 doesn't have native syntax for. So the same name seems appropriate to me - you want shadowing for the same reasons you want captures to shadow.

unittests/clangd/SyncAPI.cpp
69 ↗(On Diff #134014)

+1

I suggested capture(T&) was the right API because I thought it'd be used directly by test code (but we wrap it here) and because I thought T would be default-constructible without problems (but it's not).

I'd suggest changing to capture(Optional<T>&) instead, so the caller can just create an empty optional. That makes the callsites here slightly less obscure.

This revision is now accepted and ready to land.Feb 13 2018, 11:16 AM
ilya-biryukov marked 4 inline comments as done.
  • Use llvm::Optional<> in capture() helper to avoid confusion with llvm::Expected.

I wonder whether we want to introduce using Callback<T...> = UniqueFunction<void(T...)> for readability at some point...

I agree that's a good idea, will come up with a CL doing that.

clangd/ClangdServer.cpp
209 ↗(On Diff #134014)

I'll leave as is in this review, we have other callsites, doing the same thing, that we don't touch here. If we decide to change it, we should change all of them in one go.
I'm not opposed to the change, but like the current style a bit more.

unittests/clangd/SyncAPI.cpp
69 ↗(On Diff #134014)

The users still have to check the returned value. We only check the default-constructed value. After capture() runs Result is reassigned and now has to be checked again (and this should be done by the users).
Replaced with capture(Optional<T>&) to make it absolutely clear we're doing the right thing there.

ioeric accepted this revision.Feb 14 2018, 2:11 AM

lg

clangd/ClangdServer.cpp
209 ↗(On Diff #134014)

Feel free to land without changing this; this is a nit anyway ;)

  • Rebase onto head
This revision was automatically updated to reflect the committed changes.

Hi,

It seems that your submission broke the Windows/Linux builds for 'clangd'.

The generated error message is:

C:\Program Files (x86)\Microsoft Visual Studio 14.0\VC\include\future(263): error C2512: 'llvm::Expected<std::vector<clang::tooling::Replacement,std::allocator<_Kty>>>': no appropriate default constructor available [yyy\build\tools\clang\tools\extra\unittests\clangd\ClangdTests.vcxproj]

C:\xxx\llvm\tools\clang\tools\extra\clangd\ClangdServer.h(231): note: see declaration of 'llvm::Expected<std::vector<clang::tooling::Replacement,std::allocator<_Kty>>>'
C:\Program Files (x86)\Microsoft Visual Studio 14.0\VC\include\future(255): note: while compiling class template member function 'std::_Associated_state<_Ty>::_Associated_state(std::_Deleter_base<_Ty> *)'
C:\Program Files (x86)\Microsoft Visual Studio 14.0\VC\include\future(1491): note: see reference to function template instantiation 'std::_Associated_state<_Ty>::_Associated_state(std::_Deleter_base<_Ty> *)' being compiled
C:\Program Files (x86)\Microsoft Visual Studio 14.0\VC\include\future(989): note: see reference to class template instantiation 'std::_Associated_state<_Ty>' being compiled
C:\Program Files (x86)\Microsoft Visual Studio 14.0\VC\include\future(987): note: while compiling class template member function 'bool std::_State_manager<_Ty>::valid(void) throw() const'

Thanks