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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
