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

[Kirill Bobyrev via Phabricator via cfe-commits]

kbobyrev added inline comments.


================
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) {
----------------
kadircet wrote:
> kbobyrev wrote:
> > kadircet wrote:
> > > 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.
> > Sorry, the comment is off, it belongs to the `if` statement above. Here, the index file is already different and there is an attempt to reload it. If it succeeds, new index replaces the old one and `LastStatus` is updated.
> right and what I am saying is, even if we fail to load the the index we should still update the `LastStatus` to prevent redundant retries for a broken index file. e.g:
> 
> - For some reason a malformed index file is written with mod. time X and size Y.
> - Hot reload logic picks it up, the file exists and everything is fine.
> - When we try to read the index, we notice it is malformed or for whatever reason, deserialization fails.
> - Now we exit without updating the `LastStatus`, hence in the next update all of this will happen again even though index loading is going to fail again (as malformed index is still the same).
> 
> We could prevent that redundant loads (and failure logs) by caching the `LastStatus` as soon as the file exists.
> 
> Does that make sense now?
Ahh, I see now, makes sense. Thanks for the explanation!


================
Comment at: clang-tools-extra/clangd/index/remote/server/Server.cpp:342
+  HotReloadThread.join();
 }
----------------
kadircet wrote:
> kbobyrev wrote:
> > kadircet wrote:
> > > nit: `return 0;` ?
> > No need for that in C++.
> > 
> > https://en.cppreference.com/w/cpp/language/main_function
> > 
> > > 4) The body of the main function does not need to contain the return statement: if control reaches the end of main without encountering a return statement, the effect is that of executing `return 0;`
> right, hence the `nit`
Hm, why do you think it is better then? I'm not sure if adding no-op lines of code will increase readability/clarity.

I'm curious now, is there any reason you prefer explicit `return 0;`?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D87450/new/

https://reviews.llvm.org/D87450