[PATCH] D94554: [clangd] Add a Filesystem that overlays Dirty files.

[Sam McCall via Phabricator via cfe-commits]

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