[PATCH] D83768: [clangd] Config: Index.Background

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet added inline comments.


================
Comment at: clang-tools-extra/clangd/ConfigCompile.cpp:98
+      ValidValues.push_back(Name);
+      if (!Result && *Input == Name)
+        Result = Value;
----------------
should we assert on multiple matches of the same name ?


================
Comment at: clang-tools-extra/clangd/ConfigCompile.cpp:196
   constexpr static llvm::SourceMgr::DiagKind Error = llvm::SourceMgr::DK_Error;
+  constexpr static llvm::SourceMgr::DiagKind Warning =
+      llvm::SourceMgr::DK_Warning;
----------------
nit: I know this isn't what the patch is about, but.... what about making these lambdas/functions of Out.diag with first parameter bound to DK_Error or DK_Warning? (or having them at an anonymous namespace instead of a member)


================
Comment at: clang-tools-extra/clangd/ConfigFragment.h:124
+
+  /// Controls how clangd understands code outside the current file.
+  struct IndexBlock {
----------------
i think saying "outside the current file" might not be the best description. Maybe rather "Controls clangd's understanding for entities(symbols, refs, relations) in the codebase" ?


================
Comment at: clang-tools-extra/clangd/ConfigFragment.h:129
+    /// Legal values are "Build" or "Skip".
+    llvm::Optional<Located<std::string>> Background;
+  };
----------------
why not store the enum in here directly and generate diagnostics during the parsing stage?

we seem to be doing this for syntactic errors, e.g. `expected a dictionary` and getting an unexpected enum value sounds similar to that?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D83768