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

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet added inline comments.


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


================
Comment at: clang-tools-extra/clangd/ClangdServer.cpp:761
+  auto DiagnosticHandler = [](const llvm::SMDiagnostic &Diag) {
+    log("config {0} at {1}:{2}:{3}: {4}",
+        Diag.getKind() == llvm::SourceMgr::DK_Error ? "error" : "warning",
----------------
kadircet wrote:
> why not elog
this is still logging though :D (not elog)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D83095