Page MenuHomePhabricator

[clangd] Run formatting operations asynchronously.
ClosedPublic

Authored by sammccall on Jun 26 2020, 3:57 AM.

Details

Summary

They don't need ASTs or anything, so they should still run immediately.

These were sync for historical reasons (they predate clangd having a pervasive
threading model). This worked ok as they were "cheap".
Aside for consistency, there are a couple of reasons to make them async:

  • they do IO (finding .clang-format) so aren't trivially cheap
  • having TUScheduler involved in running these tasks means we can use it as an injection point for configuration. (TUScheduler::run will need to learn about which file is being operated on, but that's an easy change).
  • adding the config system adds more potential IO, too

Diff Detail

Event Timeline

sammccall created this revision.Jun 26 2020, 3:57 AM
Herald added a project: Restricted Project. · View Herald TranscriptJun 26 2020, 3:57 AM
kbobyrev accepted this revision.Jun 30 2020, 12:43 AM

Looks nice just a couple of nits.

clang-tools-extra/clangd/ClangdLSPServer.cpp
900

Reply(Result ? replacementsToEdits(...) : Result.takeError()) may be shorter, but up to you, maybe this looks more readable.

clang-tools-extra/clangd/ClangdServer.h
326

Is this no longer true?

This revision is now accepted and ready to land.Jun 30 2020, 12:43 AM
sammccall marked 4 inline comments as done.Jun 30 2020, 3:39 PM
sammccall added inline comments.
clang-tools-extra/clangd/ClangdLSPServer.cpp
900

Sadly it doesn't compile... the usual thing about not being able to determine a common type for the two expressions, and C++ doesn't infer it from the type that's required in the context.

clang-tools-extra/clangd/ClangdServer.h
326

It's still true that we do IO and it might be nice to cache it.
However this same IO (to get the formatstyle) also now happens in lots of other places too (without comments), and the only reason this one was especially bad is that it happened on the main thread (which is no longer true).

This revision was automatically updated to reflect the committed changes.
sammccall marked 2 inline comments as done.