sammccall added a comment. Thanks, this looks much better!
================ Comment at: clang-tools-extra/clangd/ClangdServer.cpp:434 +tweakSelection(const Range &Sel, const InputsAndAST &AST, + llvm::vfs::FileSystem *VFS = nullptr) { auto Begin = positionToOffset(AST.Inputs.Contents, Sel.start); ---------------- nit: pass nullptr explicitly rather than as a default arg. ================ Comment at: clang-tools-extra/clangd/TUScheduler.cpp:1462 +TUScheduler::getAllFileContentsFS() const { + // FIXME: Copying the files is needlessly expensive and grows according to how + // many files are being tracked, it may be worthwhile to create a Filesystem ---------------- nit: say why it's needless: usually 0-1 buffer is ever read. ================ Comment at: clang-tools-extra/clangd/TUScheduler.h:239 /// Returns a snapshot of all file buffer contents, per last update(). llvm::StringMap<std::string> getAllFileContents() const; ---------------- FIXME to remove this? ================ Comment at: clang-tools-extra/clangd/TUScheduler.h:243 + /// Returns a Filesystem snapshot of all file buffer contents, per last + /// update(). + llvm::IntrusiveRefCntPtr<llvm::vfs::FileSystem> getAllFileContentsFS() const; ---------------- I do think this is the best option, but just wanted to spell out the implications to make sure we're on the same page... - edits that occur while the action is queued will not be seen. (This is bad for the use cases we know of, but minor) - the optimization of deferring buffer copies (hinted to in the implementation) would actually break the contract, as you may see a *newer* version of the file - we don't have to worry about locking inside TUScheduler to grab the buffers ================ Comment at: clang-tools-extra/clangd/TUScheduler.h:246 + + /// Returns a Snapshot of all file buffer contents in a Filesystem overlayed + /// ontop of \p Base. ---------------- there's no need for both of these to be methods on TUScheduler, pick one? (I'd lean towards exposing this one only as it simplifies callers and has a better name) ================ Comment at: clang-tools-extra/clangd/TUScheduler.h:248 + /// ontop of \p Base. + llvm::IntrusiveRefCntPtr<llvm::vfs::OverlayFileSystem> overlayFileContents( + llvm::IntrusiveRefCntPtr<llvm::vfs::FileSystem> Base) const; ---------------- nit: take const TFS& as arg, as this function is designed for caller convenience and that's always how it'll be used ================ Comment at: clang-tools-extra/clangd/TUScheduler.h:248 + /// ontop of \p Base. + llvm::IntrusiveRefCntPtr<llvm::vfs::OverlayFileSystem> overlayFileContents( + llvm::IntrusiveRefCntPtr<llvm::vfs::FileSystem> Base) const; ---------------- sammccall wrote: > nit: take const TFS& as arg, as this function is designed for caller > convenience and that's always how it'll be used nit: return `unique_ptr<FileSystem>` (which can implicitly convert) ================ Comment at: clang-tools-extra/clangd/refactor/Tweak.h:53 + unsigned RangeEnd, SelectionTree ASTSelection, + llvm::vfs::FileSystem *VFS); /// The text of the active document. ---------------- VFS is a public field, and optional, so we could omit it from the constructor and assign it explicitly instead. ================ Comment at: clang-tools-extra/clangd/refactor/Tweak.h:69 SelectionTree ASTSelection; + /// The file system that should be queried for cross file tweaks. + llvm::vfs::FileSystem *VFS = nullptr; ---------------- let's be a bit more explicit here. ``` /// File system used to access source code (for cross-file tweaks). /// This overlays the "dirty" contents of files open in the editor, which (in case of headers) /// may not match the saved contents used for building the AST. ``` ================ Comment at: clang-tools-extra/clangd/refactor/Tweak.h:69 SelectionTree ASTSelection; + /// The file system that should be queried for cross file tweaks. + llvm::vfs::FileSystem *VFS = nullptr; ---------------- sammccall wrote: > let's be a bit more explicit here. > > ``` > /// File system used to access source code (for cross-file tweaks). > /// This overlays the "dirty" contents of files open in the editor, which (in > case of headers) > /// may not match the saved contents used for building the AST. > ``` mention that this is only populated for apply(), not prepare()? ================ Comment at: clang-tools-extra/clangd/refactor/Tweak.h:70 + /// The file system that should be queried for cross file tweaks. + llvm::vfs::FileSystem *VFS = nullptr; // FIXME: provide a way to get sources and ASTs for other files. ---------------- nit: FS rather than VFS (the "V" is already implied by the presence of the object) ================ Comment at: clang-tools-extra/clangd/refactor/tweaks/DefineOutline.cpp:406 - auto CCFile = getSourceFile(*MainFileName, Sel); + // If we have a Filesystem attached to the Selection, use that, otherwise + // fallback to the SourceManagar Filesystem. ---------------- It's not actually optional for apply, right? We shouldn't need this fallback. ================ Comment at: clang-tools-extra/clangd/tool/Check.cpp:206 + Tweak::Selection Selection(&Index, *AST, Start, End, std::move(Tree), + nullptr); for (const auto &T : prepareTweaks(Selection, Opts.TweakFilter)) { ---------------- hmm, I see. What if we make the Selection constructor fill in the default FS from AST, and then just overwrote the public field in ClangdServer::applyTweak? Then we get the FS always being set (good for tweaks), simple usage here or in tests, and the expensive copy is still explicit... Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D93978/new/ https://reviews.llvm.org/D93978 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits