[PATCH] D90775: [clangd] ExternalIndex turns off BackgroundIndex only if it isn't set

[Sam McCall via Phabricator via cfe-commits]

sammccall added a comment.

I'm not sure we should do this - it complicates the model substantially (I don't think I could explain it to a user), and it still leaves surprising cases.

I'm not sure config is the place for a lot of magic do-what-I-mean behavior - it's the place where the user overrides such behaviors.

(If you think the combinations are too confusing, we could consider *not* disabling background index implicitly for external indexes, and simply warning instead)



================
Comment at: clang-tools-extra/clangd/ConfigCompile.cpp:306
+          // Disable background indexing for the files under the mountpoint. If
+          // user didn't take any explicit actions.
+          if (C.Index.Background == Config::BackgroundPolicy::Auto)
----------------
This is vague and hard to specify more precisely.


================
Comment at: clang-tools-extra/clangd/ConfigCompile.cpp:308
+          if (C.Index.Background == Config::BackgroundPolicy::Auto)
+            C.Index.Background = Config::BackgroundPolicy::Skip;
         });
----------------
doesn't this mean that if a user has explicit `Background: Build` at a higher scope, then a more narrowly scoped external index won't disable it?

This seems at least as confusing as the problems this is trying to solve.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D90775