[PATCH] D82612: [clangd] Config: compile Fragment -> CompiledFragment -> Config

[Haojian Wu via Phabricator via cfe-commits]

hokein added inline comments.


================
Comment at: clang-tools-extra/clangd/CMakeLists.txt:41
+  ConfigCompile.cpp
   ConfigYAML.cpp
   Diagnostics.cpp
----------------
we have a few config-related files now, I wonder would it make sense to create a config/ subdir?


================
Comment at: clang-tools-extra/clangd/ConfigCompile.cpp:47
+
+  std::string RegexError = "";
+  llvm::Optional<llvm::Regex> compileRegex(const Located<std::string> &Text) {
----------------
this is only used in method `compileRegex`, looks like can be moved to a local variable? is there life-time issue?


================
Comment at: clang-tools-extra/clangd/ConfigCompile.cpp:96
+
+  constexpr static auto Error = llvm::SourceMgr::DK_Error;
+  void diag(llvm::SourceMgr::DiagKind Kind, llvm::StringRef Message,
----------------
this member seems unused.


================
Comment at: clang-tools-extra/clangd/ConfigProvider.h:32
+/// Describes the context used to evaluate configuration fragments.
+struct Params {
+  /// Absolute path to file we're targeting. Unix slashes.
----------------
nit: I find the name `Params` is too general, it implies less information. When I read the code using `Params`, I have to go-to-definition to see what it means. 

if we only have a single member in this struct, maybe just use it directly.


================
Comment at: clang-tools-extra/clangd/ConfigProvider.h:40
+///
+/// Fragments are compiled by Providers when first loaded, and cached for reuse.
+/// Like a compiled program, this is good for performance and also encourages
----------------
I might miss the context, where is the provider, not added yet? 


================
Comment at: clang-tools-extra/clangd/ConfigProvider.h:47
+  /// This always produces a usable compiled fragment (errors are recovered).
+  explicit CompiledFragment(Fragment, DiagnosticCallback);
+
----------------
Would it be more nature to have a function `compile`(or so) to do the actual compile (Fragment -> CompiledFragment) rather than doing it in a constructor?


================
Comment at: clang-tools-extra/clangd/unittests/ConfigCompileTests.cpp:40
+TEST_F(ConfigCompileTests, Condition) {
+  // No condition.
+  Frag.CompileFlags.Add.emplace_back("X");
----------------
my first impression was that each `// XXX` is a separated test, but it turns out not, the tests here seem to be stateful -- `Frag` keeps being modified. I think it is intentional, but it is hard for readers to track given that the code is long.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D82612