kadircet added a comment.
Thanks! Mostly looks good, just couple of nits.
================
Comment at: clang-tools-extra/clangd/index/remote/server/Server.cpp:223
+// whenever they become available.
+void hotReload(clangd::SwapIndex *Index, llvm::StringRef IndexPath,
+ llvm::vfs::Status &LastStatus,
----------------
Again `Index` is a non-nullptr, so let's use a ref instead.
================
Comment at: clang-tools-extra/clangd/index/remote/server/Server.cpp:231
+ // Current index is newer than the one before: no reload is needed.
+ if (NewModificationTime <= LastStatus.getLastModificationTime())
+ return;
----------------
from previous comment:
```
i think we should rather store the file_status from the original file in here.
as we want to reload whenever files size/modification_time has changed.
it doesn't have to be moving forward let alone share the same timezone
restrictions and such provided by system_clock::now.
```
================
Comment at: clang-tools-extra/clangd/index/remote/server/Server.cpp:247
+
+void runServer(clangd::SymbolIndex *Index, llvm::StringRef ServerAddress,
+ llvm::StringRef IndexPath) {
----------------
nit: maybe rename to `runServerAndWait`
================
Comment at: clang-tools-extra/clangd/index/remote/server/Server.cpp:328
+ auto Status = FS->status(IndexPath);
+ assert(Status);
+ if (!Status)
----------------
instead of both asserting and bailing out, maybe just elog before bail-out?
+ I think what you rather want is grab the inital status outside this lambda.
i.e:
```
clang::clangd::RealThreadsafeFS TFS;
auto FS = TFS...;
auto Status = ....;
if(!Status) {
// elog and exit, file does not exist
}
auto Index = ...;
if(!Index) {
// elog and exit, failed to load index
}
std::thread ..([FS = TFS.view(), LastStatus = *Status, &Index]{ ... });
runServer();
joinThread();
```
================
Comment at: clang-tools-extra/clangd/index/remote/server/Server.cpp:332
+ llvm::vfs::Status LastStatus = *Status;
+ const auto REFRESH_FREQUENCY = std::chrono::seconds(90);
+ while (!clang::clangd::shutdownRequested()) {
----------------
nit: `static constexpr auto RefreshFrequency`, i.e. use constexpr and we don't
have a different naming convention for constants. same for `WatcherFrequency`.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D87450/new/
https://reviews.llvm.org/D87450
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
