sammccall added inline comments.
================
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());
----------------
njames93 wrote:
> njames93 wrote:
> > 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.
> @sammccall Any further comments with regard to this. Is calling IO to get
> modification time in didOpen ok, or just accept opening a file would
> invalidate a preamble if the DirtyFS was used for preambles.
There's no urgency in resolving this:
- preambles-from-dirty-fs doesn't exist in this patch
- even once it does, we can introduce the problem first (in this experimental
mode) and then solve it
That said, my suspicion is that having ClangdLSPServer stat the file,
conditional on being in preambles-from-dirty-fs mode, would be OK.
(If the filesize matches I think it's safe to say the content does, and copy
the timestamp. If the filesize mismatches... well, there are a number of
scenarios involving encodings, and it's hard to say which will matter in
practice, so we'd probably want to log and wait for the bug reports)
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
