[PATCH] D88172: [clangd] Extract common file-caching logic from ConfigProvider.

[Kirill Bobyrev via Phabricator via cfe-commits]

kbobyrev added a comment.

Sorry for such a tremendous delay.

Mostly looks good functionally, I just have a few questions about semantics and several stylistic nits.

Also, thanks for the amount of documentation - it makes it really nice to navigate around and review.



================
Comment at: clang-tools-extra/clangd/support/FileCache.cpp:18
+// The cached value does not reflect the current content on disk.
+static constexpr uint64_t CacheDiskMismatch = -1;
+// The cached value reflects that the file doesn't exist.
----------------
Uhhh. Doesn't Clang-Tidy think this is a bad idea? IIUC this is not UB but it looks quite scary regardless. Maybe use `std::numeric_limits<uint64_t>::max()` here?


================
Comment at: clang-tools-extra/clangd/support/FileCache.cpp:44
+  auto BumpValidTime = llvm::make_scope_exit(
+      [&] { ValidTime = std::chrono::steady_clock::now(); });
+
----------------
Wait, I thought `ValidTime` is the last time point when the file could be correctly parsed (i.e. the file was valid). But if the parsing could not be done then this would be bumped regardless, right? So what's the "validity" semantics here?


================
Comment at: clang-tools-extra/clangd/support/FileCache.h:23
+///
+/// We want configuration files to be "live" as much as possible.
+/// Reading them every time is simplest, but caching solves a few problems:
----------------
s/live/fresh?


================
Comment at: clang-tools-extra/clangd/support/FileCache.h:58
+  void read(const ThreadsafeFS &TFS,
+            std::chrono::steady_clock::time_point FreshTime,
+            llvm::function_ref<void(llvm::Optional<llvm::StringRef>)> Parse,
----------------
So the client is responsible for keeping `FreshTime` updated, right? I think it might be worth mentioning in the comments.


================
Comment at: clang-tools-extra/clangd/support/FileCache.h:70
+  mutable std::chrono::steady_clock::time_point ValidTime;
+  mutable llvm::sys::TimePoint<> MTime;
+  mutable uint64_t Size;
----------------
I assume `MTime` is `ModifiedTime` - took me some time to realize it (getting into the code), maybe expand or add some comment. Also, why are the types different with `ValidTime`?


================
Comment at: clang-tools-extra/clangd/unittests/support/FileCacheTests.cpp:15
+#include <atomic>
+#include <chrono>
+
----------------
Please sort the includes here (clangd should have a Code Action here).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88172