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

[Nathan James via Phabricator via cfe-commits]

njames93 marked 11 inline comments as done.
njames93 added inline comments.


================
Comment at: clang-tools-extra/clangd/ClangdServer.h:389
+
+  class DirtyFS : public ThreadsafeFS {
+  public:
----------------
sammccall wrote:
> njames93 wrote:
> > Probably needs moving to its own place, but wasn't sure where best to put it.
> I think ClangdServer is fine, but this can be a unique_ptr<ThreadsafeFS> with the class moved to the implementation file
How about privately inheriting from ThreadsafeFS :P
I agree, hiding the impl in the cpp is probably a much nicer way.


================
Comment at: clang-tools-extra/clangd/DraftStore.cpp:80
 
-  Draft &D = Drafts[File];
-  updateVersion(D, Version);
-  D.Contents = Contents.str();
-  return D.Version;
+  auto Res = Drafts.try_emplace(File);
+  auto &D = Res.first->getValue();
----------------
sammccall wrote:
> we don't need try_emplace here, we overwrite the whole value regardless.
> Can we use `Draft &D = Drafts[File]` again?
I changed this because in an older version I wanted to set the UniqueID on the first time the file was added, however now we don't do that (or check the return value for insertion at all) its safe to go back to the `operator[]` access.


================
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:
> 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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D94554/new/

https://reviews.llvm.org/D94554