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

[Sam McCall via Phabricator via cfe-commits]

sammccall added a comment.

Sorry for the long delay here. I've gotten myself buried in too many different things.

Overall the idea of exposing drafts as an overlay FS is definitely growing on me - it makes it easier and clearer to control which feature is seeing which version of the files.

At a high level my main requests are:

- we should move maintenance of the drafts from ClangdLSPServer into ClangdServer. (This should maybe have been done a while ago, but this change makes it more complicated and therefore pressing)
- we should simplify the implementation as much as possible, and lean on existing infrastructure - there's a lot of custom pieces here like RefCntString and DraftStoreFileSystem that seem like small optimizations over existing types

(I might take a crack at moving DraftStore by itself, as there probably are issues and I want to understand what they are...)



================
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);
----------------
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)


================
Comment at: clang-tools-extra/clangd/DraftStore.cpp:145
+
+class RefCntStringMemBuffer : public llvm::MemoryBuffer {
+public:
----------------
These classes need comments describing why they exist :-)
(I think you're right that buffers may outlive the VFS)


================
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
----------------
Is there any reason to think we need this micro-optimization? It's quite invasive.


================
Comment at: clang-tools-extra/clangd/DraftStore.cpp:213
+
+class DraftStoreFileSystem : public llvm::vfs::FileSystem {
+public:
----------------
can't we use InMemoryFileSystem for this?


================
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());
----------------
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)


================
Comment at: clang-tools-extra/clangd/DraftStore.h:25
+/// A Reference counted string that is used to hold the contents of open files.
+class RefCntString : public llvm::ThreadSafeRefCountedBase<RefCntString> {
+public:
----------------
shared_ptr<const string> seems like a cheap alternative here.

However if the point is ultimately to obtain MemoryBuffers that we can hand out with the underlying data being refcounted, can we do that directly?

```
class SharedBuffer : public MemoryBuffer {
  std::shared_ptr<const std::string> Data;
public:
  Shared(StringRef Name, std::string Data);
  Shared(const RefcountBuffer &); // copyable
};
```


================
Comment at: clang-tools-extra/clangd/DraftStore.h:31
+
+  operator const std::string &() const { return Data; }
+  operator StringRef() const { return str(); }
----------------
providing non-explicit string conversion operators is IME very likely to cause ambiguous overload errors when using this in variaous places :-(


================
Comment at: clang-tools-extra/clangd/DraftStore.h:82
+  const ThreadsafeFS &getDraftFS() const {
+    assert(DraftFS && "No draft fs has been set up");
+    return *DraftFS;
----------------
avoid assert in headers for ODR reasons (ooh, we have a couple of violations i should clean up)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D94554