kadircet added inline comments.
================
Comment at: clang-tools-extra/clangd/index/remote/server/Server.cpp:71
std::unique_ptr<clangd::SymbolIndex> openIndex(llvm::StringRef Index) {
return loadIndex(Index, /*UseIndex=*/true);
----------------
why do we have this extra indirection?
================
Comment at: clang-tools-extra/clangd/index/remote/server/Server.cpp:86
+ clangd::SwapIndex *Index;
+
----------------
why not make this private? also keep using `SymbolIndex`. also you can make
this a reference now.
================
Comment at: clang-tools-extra/clangd/index/remote/server/Server.cpp:224
+// whenever they become available.
+void hotReload(clangd::SwapIndex **Index, llvm::StringRef IndexPath) {
+ llvm::sys::TimePoint<> LastModificationTime =
----------------
why is this taking a double pointer? I think you can just use a reference here
for `Index` (i.e. `clangd::SwapIndex &`
================
Comment at: clang-tools-extra/clangd/index/remote/server/Server.cpp:226
+ llvm::sys::TimePoint<> LastModificationTime =
+ std::chrono::system_clock::now();
+ for (;; std::this_thread::sleep_for(std::chrono::seconds(90))) {
----------------
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.
Also it would be nice to use a VFS in here, that way we can move this logic
into a helper in the future if other components need similar logic.
================
Comment at: clang-tools-extra/clangd/index/remote/server/Server.cpp:227
+ std::chrono::system_clock::now();
+ for (;; std::this_thread::sleep_for(std::chrono::seconds(90))) {
+ llvm::sys::fs::file_status Status;
----------------
how's this loop terminated?
I would suggest factoring the loop body into a helper and then having a lambda
for periodically running it, while looking for an exit signal.
================
Comment at: clang-tools-extra/clangd/index/remote/server/Server.cpp:263
+ std::thread HotReloadThread(hotReload, &Service.Index, IndexPath);
+
----------------
i think `hotReload` thread creation should also be part of the main.
================
Comment at: clang-tools-extra/clangd/index/remote/server/Server.cpp:266
Server->Wait();
}
----------------
you either need to `join` a thread before its destruction or `detach` it from
the main thread.
================
Comment at: clang-tools-extra/clangd/index/remote/server/Server.cpp:313
- std::unique_ptr<clang::clangd::SymbolIndex> Index = openIndex(IndexPath);
+ std::unique_ptr<clang::clangd::SwapIndex> Index(
+ new clang::clangd::SwapIndex(openIndex(IndexPath)));
----------------
nit: `auto Index = std::make_unique<clang::clangd::SwapIndex>(openIndex...);`
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D87450/new/
https://reviews.llvm.org/D87450
_______________________________________________
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
