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

[Nathan James via Phabricator via cfe-commits]

njames93 marked 9 inline comments as done.
njames93 added inline comments.


================
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:
> 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)
I think the flexibility of passing a `Filesystem` is better than the `ThreadsafeFS`.


================
Comment at: clang-tools-extra/clangd/refactor/Tweak.h:53
+              unsigned RangeEnd, SelectionTree ASTSelection,
+              llvm::vfs::FileSystem *VFS);
     /// The text of the active document.
----------------
sammccall wrote:
> VFS is a public field, and optional, so we could omit it from the constructor and assign it explicitly instead.
We could, but it would messy code elsewhere.


================
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)) {
----------------
sammccall wrote:
> 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...
I've gone with an approach that the FS gets passed in the constructor, if nullptr is passed then we use the fallback, helpful for tests


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D93978