sammccall accepted this revision.
sammccall added a comment.
This revision is now accepted and ready to land.
Herald added a project: clang-tools-extra.
Sorry, I thought this had already gone in!
LG, sorry for the back and forth.
================
Comment at: clang-tools-extra/clangd/ClangdServer.cpp:575
return CB(InpAST.takeError());
- auto Selections = tweakSelection(Sel, *InpAST);
+ // FIXME: Should we use the dirty fs here?
+ auto FS = TFS.view(llvm::None);
----------------
njames93 wrote:
> Regarding this, Is it wise to set the contract so that Tweak::prepare isn't
> allowed to do IO so we should just pass a nullptr here, inline with how
> prepareRename works.
Yes, I think so.
================
Comment at: clang-tools-extra/clangd/refactor/Tweak.cpp:54
+ SelectionEnd(RangeEnd), ASTSelection(std::move(ASTSelection)),
+ FS(FS ? FS
+ : &AST.getSourceManager().getFileManager().getVirtualFileSystem())
{
----------------
I'm no longer convinced this fallback is a good idea.
We want prepare() to do no IO, ideally we'd assert if we do. But by falling
back to the AST FS, we'll mask that problem.
There are only two callsites that need this fallback (TweakTesting & Check) - I
think we should just inline what we mean there.
================
Comment at: clang-tools-extra/clangd/refactor/tweaks/DefineOutline.cpp:67
+ const Tweak::Selection &Sel,
+ llvm::vfs::FileSystem *FS) {
+ if (auto Source = getCorrespondingHeaderOrSource(FileName, FS))
----------------
why take this as an explicit parameter rather than just accessing (and
asserting) Sel.FS?
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
