kadircet added a comment.
oops, looks like i forgot to hit submit in the morning..
================
Comment at: clang-tools-extra/clangd/index/remote/server/Server.cpp:228
+ auto Status = FS->status(IndexPath);
+ if (!Status || Status->equivalent(LastStatus))
+ return;
----------------
i beleive `equivalent` checks for inode number equivalence (at least on linux,
using realfilesystem). That's not guranteed to change when you overwrite/modify
a file.
Sorry if it felt like i meant this, but I was literally suggesting to use `!=`
instead of `<=` when comparing modifcation time, and also incorporating size
into the check.
Also please let me know if there's something i am missing with the `equivalent`.
It would be also nice to have some tests, as we are starting to add non-trivial
functionality to server but not blocking for this patch. (i suppose it would've
catched the equivalent problem)
================
Comment at: clang-tools-extra/clangd/index/remote/server/Server.cpp:234
+ LastStatus.getLastModificationTime(),
Status->getLastModificationTime());
+ std::unique_ptr<clang::clangd::SymbolIndex> NewIndex = loadIndex(IndexPath);
+ if (!NewIndex) {
----------------
i think we should update `LastStatus` here, as we are going to fail loading the
index unless the file changes. so there's no need to retry loading the index if
the file hasn't changed.
================
Comment at: clang-tools-extra/clangd/index/remote/server/Server.cpp:236
+ if (!NewIndex) {
+ vlog("Failed to load new index. Old index will be served.");
+ return;
----------------
i believe, this should be an `elog`.
================
Comment at: clang-tools-extra/clangd/index/remote/server/Server.cpp:240
+ Index.reset(std::move(NewIndex));
+ log("New index version loaded. Last modification time: {0}.",
+ Status->getLastModificationTime());
----------------
nit: maybe also print the size.
================
Comment at: clang-tools-extra/clangd/index/remote/server/Server.cpp:318
+ if (!Status) {
+ elog("File {0} does not exist.", IndexPath);
+ return Status.getError().value();
----------------
nit: drop `File`
================
Comment at: clang-tools-extra/clangd/index/remote/server/Server.cpp:335
+ std::this_thread::sleep_for(RefreshFrequency);
+ hotReload(*Index, llvm::StringRef(IndexPath), LastStatus, FS);
+ }
----------------
nit: i would first call `hotReload` and then sleep to get rid of one extra
reload of the index when we are shutting down.
================
Comment at: clang-tools-extra/clangd/index/remote/server/Server.cpp:342
+ HotReloadThread.join();
}
----------------
nit: `return 0;` ?
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
