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

Kadir Cetinkaya via Phabricator via cfe-commits cfe-commits at lists.llvm.org
Wed Sep 16 01:36:17 PDT 2020


kadircet 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) {
----------------
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?


================
Comment at: clang-tools-extra/clangd/index/remote/server/Server.cpp:342
+  HotReloadThread.join();
 }
----------------
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`


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D87450



More information about the cfe-commits mailing list