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

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet added a comment.

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



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


================
Comment at: clang-tools-extra/clangd/ClangdServer.cpp:756
+    llvm::SmallString<256> PosixPath = File;
+    llvm::sys::path::native(PosixPath);
+    Params.Path = PosixPath.str();
----------------
also provide posix style instead of defaulted native


================
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",
----------------
why not elog


================
Comment at: clang-tools-extra/clangd/ClangdServer.h:340
+  Context createProcessingContext(PathRef) const;
+  config::Provider *ConfigProvider;
+
----------------
nit: `= nullptr;`


================
Comment at: clang-tools-extra/clangd/index/Background.h:142
+      std::function<void(BackgroundQueue::Stats)> OnProgress = nullptr,
+      std::function<Context(PathRef)> ContextProvider = nullptr);
   ~BackgroundIndex(); // Blocks while the current task finishes.
----------------
doesn't need to be in this patch, bu I think it is time we have an opts struct in here.


================
Comment at: clang-tools-extra/clangd/unittests/TUSchedulerTests.cpp:879
   WithContextValue CtxWithKey(TestKey, 10);
-  S.run("props context", [&] {
+  S.run("props context", "somepath", [&] {
     EXPECT_EQ(Context::current().getExisting(TestKey), 10);
----------------
nit: maybe extract "somepath" to a constant


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D83095