[PATCH] D93978: [clangd] DefineOutline doesn't require implementation file being saved

[Sam McCall via Phabricator via cfe-commits]

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