[PATCH] D87450: [clangd] Implement hot index reloading for clangd-index-server

[Kadir Cetinkaya via Phabricator via cfe-commits]

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