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