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

Wed Nov 25 02:19:55 PST 2020

sammccall added inline comments.

================
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,
----------------
kbobyrev wrote:
> So the client is responsible for keeping `FreshTime` updated, right? I think it might be worth mentioning in the comments.
I'm not really following - FreshTime is a parameter and must be provided each time, so I'm not sure what "keep it updated" means.

It's true that clients likely want to pass `now() + some_duration`, I've added an example explaining that to the docs.
But I don't think "if you store the value of now() for a long time and use it later, it may refer to a time way in the past" isn't a trap we need to warn against.

================
Comment at: clang-tools-extra/clangd/unittests/support/FileCacheTests.cpp:15
+#include <atomic>
+#include <chrono>
+
----------------
kbobyrev wrote:
> njames93 wrote:
> > kbobyrev wrote:
> > > Please sort the includes here (clangd should have a Code Action here).
> > The includes are sorted, this is a bug in the include order check that was fixed in D91602, but the pre merge bot isnt using the updated version.
> Ahh, I didn't know that and I had some of the valid warnings in my patches. I see, thanks!
I can apply the fix, but then `arc diff` wants to revert it. I don't think this is important enough to fight the tools over.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88172