[PATCH] D82964: [clangd] Config: loading and caching config from disk.

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet added a comment.

thanks looks really cool!



================
Comment at: clang-tools-extra/clangd/ConfigProvider.cpp:39
+            std::vector<CompiledFragment> &Out) {
+    assert(llvm::sys::path::is_absolute(Path));
+    auto FS = TFS.view(/*CWD=*/llvm::None);
----------------
I believe this function is going to be hot, I would suggest putting a span here to track the latencies


================
Comment at: clang-tools-extra/clangd/ConfigProvider.cpp:41
+    auto FS = TFS.view(/*CWD=*/llvm::None);
+    auto Stat = FS->status(Path);
+    if (!Stat || !Stat->isRegularFile()) {
----------------
i suppose doing this IO on most of our features are OK, as they tend to search for .clang-format at least (even the ancestor traversal), but I wonder whether these will also be triggered for code completion and signature help, as they currently don't do any IO (apart from preamble deserialization, and maybe one patched include file).

Not a concern for this patch, but something to keep in mind when integrating with the rest.


================
Comment at: clang-tools-extra/clangd/ConfigProvider.cpp:57
+        // For simplicity, don't cache the value in this case (use a bad key).
+        if (Buf->get()->getBufferSize() != Size) {
+          Size = -1;
----------------
is this really necessary ? we are setting the cache key to old stat values anyway, so the next read shouldn't match neither the Size nor MTime.


================
Comment at: clang-tools-extra/clangd/ConfigProvider.cpp:67
+    }
+    llvm::copy(CachedValue, std::back_inserter(Out));
+  }
----------------
nit: I've found the nestedness a bit overwhelming, wdyt about putting this into a scope_exit function and using early returns above ?


================
Comment at: clang-tools-extra/clangd/ConfigProvider.cpp:120
+           I != E; ++I) {
+        // Avoid weird non-substring cases like phantom "." components.
+        if (I->end() < Parent.begin() && I->end() > Parent.end())
----------------
ummm, what ? is this a bug in path iterator that should be fixed rather than worked around?


================
Comment at: clang-tools-extra/clangd/ConfigProvider.cpp:181
+  for (const auto &Fragment : getFragments(P, DC))
+    Fragment(P, C);
+  return C;
----------------
nit: I think this looks a bit ... surprising


================
Comment at: clang-tools-extra/clangd/ConfigProvider.h:72
+
+  /// A provider that includes fragments from all the supplied providers.
+  static std::unique_ptr<Provider>
----------------
maybe also mention precedence order ? I suppose it already has the natural, the latter one will override the former behavior, but saying out explicitly wouldn't hurt.


================
Comment at: clang-tools-extra/clangd/ConfigProvider.h:74
+  static std::unique_ptr<Provider>
+      combine(std::vector<std::unique_ptr<Provider>>);
+
----------------
maybe `chain` instead of `combine` ?


================
Comment at: clang-tools-extra/clangd/ConfigProvider.h:85
+  /// When parsing/compiling, the DiagnosticCallback is used to report errors.
+  /// Usual thread-safety guarantees apply: this function must be threadsafe.
+  virtual std::vector<CompiledFragment>
----------------
first part of this sentence doesn't say much, maybe drop it?


================
Comment at: clang-tools-extra/clangd/ConfigProvider.h:91
+/// A provider that reads config from a YAML file in ~/.config/clangd.
+std::unique_ptr<Provider> createFileConfigProvider(const ThreadsafeFS &);
+
----------------
doesn't seem to be implemented anywhere? I suppose it is just a convenience wrapper around `Provider::fromYAMLFile`, is it worth it?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D82964