[PATCH] D98029: [clangd] Introduce a CommandLineConfigProvider

[Sam McCall via Phabricator via cfe-commits]

sammccall accepted this revision.
sammccall added a comment.
This revision is now accepted and ready to land.

Both side-effects seem fine to me.

> EnableBackgroundIndex option controls whether the component will be created at all

Do you think we should eventually switch to always setting ClangdServer::Options::BackgroundIndex to true in ClangdServerMain (or removing the option?)
If not, I'm not sure if there's much benefit in also setting the config flag...

> CompileCommandsDir is also used by clangd --check workflow, which doesn't use configs

We can/should fix this though, right?



================
Comment at: clang-tools-extra/clangd/tool/ClangdMain.cpp:555
+    };
+    if (Sync) {
+      IndexLoadTask();
----------------
(This seems a little weird, get passed in a task runner and maybe ignore it. Seems like the asyncprojectindex should know whether it's supposed to be sync or not, and either pass in a null runner or it should handle the asynchrony itself... not something for this patch)


================
Comment at: clang-tools-extra/clangd/tool/ClangdMain.cpp:566
+
+class CommandLineFlagsConfigProvider : public config::Provider {
+  std::vector<config::CompiledFragment>
----------------
or just FlagsConfigProvider?


================
Comment at: clang-tools-extra/clangd/tool/ClangdMain.cpp:570
+               config::DiagnosticCallback) const override {
+    config::CompiledFragment Frag = [](const config::Params &, Config &C) {
+      if (!CompileCommandsDir.empty()) {
----------------
Hmm, it seems natural to move assembling some of these objects into the constructor, I'm not sure it matters much, at least for now, but putting the actual flag-inspection on the hot-ish path seems constraining.

(Lowest-ceremony I can come up with is just making them e.g. Optional<ExternalIndexSpec> members of the provider, and then having a single fragment which capture the provider `this` and keep the conditionals inside it)


================
Comment at: clang-tools-extra/clangd/tool/ClangdMain.cpp:756
+        CompileCommandsDir = Path.str().str();
+        // FIXME: Still set for clangd --check. Use config in --check workflow
+        // and get rid of these options?
----------------
If we fix this, we can move the rest of the logic into the flags config provider constructor.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D98029