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