[PATCH] D83095: [clangd] Config: compute config in TUScheduler and BackgroundIndex

[Sam McCall via Phabricator via cfe-commits]

sammccall marked 2 inline comments as done.
sammccall added inline comments.


================
Comment at: clang-tools-extra/clangd/ClangdServer.cpp:748
 
+Context ClangdServer::createProcessingContext(PathRef File) const {
+  if (!ConfigProvider)
----------------
kadircet wrote:
> sammccall wrote:
> > kadircet wrote:
> > > 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
> > 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.
> 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.
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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D83095/new/

https://reviews.llvm.org/D83095