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

[Sam McCall via Phabricator via cfe-commits]

sammccall marked 5 inline comments as done.
sammccall added inline comments.


================
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.
----------------
kbobyrev wrote:
> 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?
Done.
(FWIW I prefer the -1 idiom as you don't have to repeat the type so there's no chance of a mismatch)


================
Comment at: clang-tools-extra/clangd/support/FileCache.cpp:44
+  auto BumpValidTime = llvm::make_scope_exit(
+      [&] { ValidTime = std::chrono::steady_clock::now(); });
+
----------------
kbobyrev wrote:
> 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?
Ah, "valid" refers to the cache, not the file. The cache is valid if it represents what's on disk, whether that's good, bad or missing. (Note that this class doesn't actually know anything about the semantics of the file content, or whether it's valid).

Added a comment to the member.


================
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:
----------------
kbobyrev wrote:
> s/live/fresh?
I guess it depends which way you look at it:
You want the *configuration file* to be "live" in the sense that it takes immediate effect.
You want the *configuration* to be "fresh" in the sense that it reflects the latest source from somewhere.


================
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;
----------------
kbobyrev wrote:
> 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`?
Renamed to ModifiedTime. (mtime is a term of art: see mtime/ctime/atime. But spelling it out is nearly as precise and less cryptic).

The types are different because they are different coordinate systems: steady_clock is approximately "nanos since boot" which is the best way to compare timestamps captured within a program, and system_clock has semantics that match mtime stored in the filesystem (though actually we just compare for equality, so it's just the most convenient type).

Changed ModifiedTime to spell the underlying std type directly to make this difference clearer (though the API we get it from uses the LLVM typedef).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88172