This is an archive of the discontinued LLVM Phabricator instance.

[clangd] Config: compute config in TUScheduler and BackgroundIndex
ClosedPublic

Authored by sammccall on Jul 2 2020, 2:12 PM.

Details

Summary

ClangdServer owns the question of exactly which config to create, but
TUScheduler/BackgroundIndex control threads and so decide at which point
to inject it.

Diff Detail

Event Timeline

sammccall created this revision.Jul 2 2020, 2:12 PM
Herald added a project: Restricted Project. Β· View Herald TranscriptJul 2 2020, 2:12 PM

LG, mostly nits apart from a question about moving the context generation logic into a different place.

clang-tools-extra/clangd/ClangdServer.cpp
748

why not make this a free function in ConfigProvider.h ?

that way we could just keep passing ConfigProvider around rather than a derived lambda.
It makes testing more cumbersome, but enables us to move the logic out of ClangdServer

756

also provide posix style instead of defaulted native

761

why not elog

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

nit: = nullptr;

clang-tools-extra/clangd/index/Background.h
142

doesn't need to be in this patch, bu I think it is time we have an opts struct in here.

clang-tools-extra/clangd/unittests/TUSchedulerTests.cpp
879–880

nit: maybe extract "somepath" to a constant

sammccall updated this revision to Diff 275387.Jul 3 2020, 7:05 AM
sammccall marked 6 inline comments as done.

address comments

clang-tools-extra/clangd/ClangdServer.cpp
748

Oh, I thought hiding this logic in ClangdServer was a feature!

For instance, if we want to report configuration errors as LSP diagnostics or as notifications, we need to have access to the ClangdServer to do that.

clang-tools-extra/clangd/index/Background.h
142

Yeah, makes sense. (But indeed would rather not do it here...)

sammccall updated this revision to Diff 275388.Jul 3 2020, 7:07 AM

fix -Wreorder

kadircet added inline comments.Jul 3 2020, 12:20 PM
clang-tools-extra/clangd/ClangdServer.cpp
748

hmm, I thought we would rather make DiagnosticHandler an input parameter in such a scenario, (we even have the alias config::DiagnosticCallback πŸ˜…) with a nullptr just logging the errors and warnings (as in current implementation).

I don't see any other interaction but surfacing diagnostics, and I believe it would look nicer if we provided a callback for diags when creating context with configs. But this one also gets the job done, so up to you.

761

this is still logging though :D (not elog)

kadircet accepted this revision.Jul 3 2020, 12:23 PM

thanks, lgtm!

This revision is now accepted and ready to land.Jul 3 2020, 12:23 PM
sammccall marked 2 inline comments as done.Jul 4 2020, 2:14 AM
sammccall added inline comments.
clang-tools-extra/clangd/ClangdServer.cpp
748

If i'm understanding what you mean (pass a callback in clangdserver::options?) I don't think it's the right level of abstraction. E.g. for diagnostics, we do have a lifecycle and data model around diagnostics that clangdserver is responsible for producing, and delivering raw diagnostics one at a time doesn't seem enough.
Similarly if we want warning/error notifications that seems like a higher level thing to add to clangdserver::callbacks.
(If it's the ClangdServer builder's responsibility to handle the errors, then the ConfigProvider interface isn't quite the right one in ClangdServer::Options - why expose the errors to clangdserver if they're just going to be passed back again?)

Anyway, we probably can't know for sure until we come to do richer error reporting, which isn't a top priority at the moment. This is my best guess for where we should handle errors (or at least translate them) but I don't think it's terribly hard to move later. So I'll leave as-is for now, to unblock the more critical parts of this patch.

This revision was automatically updated to reflect the committed changes.