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

[Nathan James via Phabricator via cfe-commits]

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