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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
