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

[Nathan James via Phabricator via cfe-commits]

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


================
Comment at: clang-tools-extra/clangd/ClangdServer.h:159
   ClangdServer(const GlobalCompilationDatabase &CDB, const ThreadsafeFS &TFS,
-               const Options &Opts, Callbacks *Callbacks = nullptr);
+               const ThreadsafeFS &DirtyFS, const Options &Opts,
+               Callbacks *Callbacks = nullptr);
----------------
sammccall wrote:
> Passing in FS + DirtyFS doesn't seem quite right, because we also pass in all the mutations that create the differences.
> 
> It seem rather that *ClangdServer* should maintain the overlay containing the dirty buffers, and expose it (for use in the few places that ClangdLSPServer currently uses DraftStore).
> 
> (the fact that DraftStore isn't accessible from ClangdServer did also come up when trying to split the *Server classes into Modules, so this seems like a helpful direction from that perspective too)
I was never a fan of this tbh. But moving DraftStore would result in this being too big,


================
Comment at: clang-tools-extra/clangd/DraftStore.cpp:148
+  static std::unique_ptr<RefCntStringMemBuffer>
+  create(IntrusiveRefCntPtr<RefCntString> Data, StringRef Name) {
+    // Allocate space for the FileContentMemBuffer and its name with null
----------------
sammccall wrote:
> Is there any reason to think we need this micro-optimization? It's quite invasive.
Its basically copied from llvm/support/MemoryBuffer.cpp


================
Comment at: clang-tools-extra/clangd/DraftStore.cpp:213
+
+class DraftStoreFileSystem : public llvm::vfs::FileSystem {
+public:
----------------
sammccall wrote:
> can't we use InMemoryFileSystem for this?
`InMemoryFileSystem` Would work in theory. However using that requires building a lot of unnecessary nodes for thing like intermediate directories.


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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D94554