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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
