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

[Sam McCall via Phabricator via cfe-commits]

sammccall added inline comments.


================
Comment at: clang-tools-extra/clangd/ConfigCompile.cpp:308
+          if (C.Index.Background == Config::BackgroundPolicy::Auto)
+            C.Index.Background = Config::BackgroundPolicy::Skip;
         });
----------------
kadircet wrote:
> sammccall wrote:
> > 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.
> > 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?
> 
> Yes. The motivating case I had was something like this:
> - Let's say user turns background-index on inside `llvm-project/clang-tools-extra/clangd/.clangd`.
> - Defines an external index via user-config for `llvm-project`.
> 
> Since user-config is the inner-most provider, it will turn off the backgrond-indexing implicitly for the whole project, and user won't be able to turn it on for a subdirectory without specifying that in user-config directory too.
In that scenario, I'm not sure whether the user wants background indexing on or not. (e.g. more likely *someone else* turned it on in the project config, who wasn't aware of the user config).

Maybe there's a principle here that project config for an inner directory is actually more specific and therefore should take precedence over user config for an outer directory? That seems like it might be at least as useful as the current behavior, but we shouldn't be implementing it setting-by-setting.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D90775