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