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

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet accepted this revision.
kadircet marked an inline comment as done.
kadircet added a comment.
This revision is now accepted and ready to land.

let's ship it!



================
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;
----------------
sammccall wrote:
> 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?
SG :+1:


================
Comment at: clang-tools-extra/clangd/ConfigFragment.h:129
+    /// Legal values are "Build" or "Skip".
+    llvm::Optional<Located<std::string>> Background;
+  };
----------------
sammccall wrote:
> 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?
sure


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D83768