njames93 added inline comments.
================
Comment at: clang-tools-extra/clangd/ClangdServer.h:389
+
+ class DirtyFS : public ThreadsafeFS {
+ public:
----------------
Probably needs moving to its own place, but wasn't sure where best to put it.
================
Comment at: clang-tools-extra/clangd/DraftStore.cpp:287
+ for (const auto &KV : DS.Drafts) {
+ // Query the base filesystem for file uniqueids.
+ auto BaseStatus = BaseView->status(KV.getKey());
----------------
sammccall wrote:
> njames93 wrote:
> > sammccall wrote:
> > > doing IO in view() doesn't seem obviously OK.
> > >
> > > what's the practical consequence of the overlay's inodes not matching
> > > that of the underlying FS? (It seems reasonable to say that the files
> > > always have different identity, and may/may not have the same content)
> > It's probably not necessary, but I know preambles use the UniqueID. It may
> > just be safer to create a uniqueID for each item in the DraftStore and
> > leave it at that.
> The biggest risks I can see with this approach:
> - in build-preamble-from-dirty-FS mode, would opening a draft cause the
> preamble to be considered invalid and needing rebuilding? My read of
> PrecompiledPreamble::CanReuse is no (UniqueIDs are not stored)
> - when parsing, if the file can be read through the overlay *and* underlying
> via different paths (e.g. hardlinks), these won't be deduplicated. However
> giving them the same inode only changes the nature of the bug: the file would
> get whichever content was read first.
>
> So I think it's as good as we can expect.
Unfortunately in the current implementation, opening a draft will invalidate
and preambles that use the underlying file.
This is due to the last modification times not matching.
This could be addressed by querying the TFS on the textDocument/didOpen request
to get the actual last modification time, but we shouldn't really be doing any
kind of IO on that thread.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D94554/new/
https://reviews.llvm.org/D94554
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
