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

[Sam McCall via Phabricator via cfe-commits]

sammccall added inline comments.


================
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;
----------------
kadircet wrote:
> 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)
I'm not sure about the readability of lambdas here, if we wanted to do this I think the simplest thing would just be to make them methods?

Can we defer this question until after the branch cut?


================
Comment at: clang-tools-extra/clangd/ConfigFragment.h:124
+
+  /// Controls how clangd understands code outside the current file.
+  struct IndexBlock {
----------------
kadircet wrote:
> 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" ?
I don't really understand this - clangd's understanding for symbols, refs and, relations is all based on the AST, except when we're talking about outside the current file. (And refs and relations aren't terms that make much sense to users)

Expanded this a bit and gave an example.


================
Comment at: clang-tools-extra/clangd/ConfigFragment.h:129
+    /// Legal values are "Build" or "Skip".
+    llvm::Optional<Located<std::string>> Background;
+  };
----------------
kadircet wrote:
> 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?
Because then you have to do it twice for JSON vs YAML. You're right this isn't really principled, and I think more data points will help us resolve it.
OK to defer this to after the branch cut?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D83768