diff --git a/clang-tools-extra/clangd/refactor/tweaks/DefineOutline.cpp b/clang-tools-extra/clangd/refactor/tweaks/DefineOutline.cpp --- a/clang-tools-extra/clangd/refactor/tweaks/DefineOutline.cpp +++ b/clang-tools-extra/clangd/refactor/tweaks/DefineOutline.cpp @@ -386,12 +386,9 @@ } bool prepare(const Selection &Sel) override { - // Bail out if we are not in a header file. - // FIXME: We might want to consider moving method definitions below class - // definition even if we are inside a source file. - if (!isHeaderFile(Sel.AST->getSourceManager().getFilename(Sel.Cursor), - Sel.AST->getLangOpts())) - return false; + bool IsHeader = + isHeaderFile(Sel.AST->getSourceManager().getFilename(Sel.Cursor), + Sel.AST->getLangOpts()); Source = getSelectedFunction(Sel.ASTSelection.commonAncestor()); // Bail out if the selection is not a in-line function definition. @@ -399,8 +396,15 @@ Source->isOutOfLine()) return false; + // If we are in a source file and the function is a free function, bail out. + if (!isa(Source) && !IsHeader) + return false; + + IsCrossFileEdit = IsHeader; + // Bail out if this is a function template or specialization, as their // definitions need to be visible in all including translation units. + // FIXME: This can be supported if we are not making a cross file edit. if (Source->getDescribedFunctionTemplate()) return false; if (Source->getTemplateSpecializationInfo()) @@ -430,19 +434,33 @@ Expected apply(const Selection &Sel) override { const SourceManager &SM = Sel.AST->getSourceManager(); - auto CCFile = getSourceFile(Sel.AST->tuPath(), Sel); - - if (!CCFile) - return error("Couldn't find a suitable implementation file."); - assert(Sel.FS && "FS Must be set in apply"); - auto Buffer = Sel.FS->getBufferForFile(*CCFile); - // FIXME: Maybe we should consider creating the implementation file if it - // doesn't exist? - if (!Buffer) - return llvm::errorCodeToError(Buffer.getError()); - auto Contents = Buffer->get()->getBuffer(); - auto InsertionPoint = getInsertionPoint( - Contents, Source->getQualifiedNameAsString(), Sel.AST->getLangOpts()); + + std::optional OwnedPath; + std::unique_ptr OwnedBuffer; + PathRef Path; + StringRef TargetContents; + if (IsCrossFileEdit) { + OwnedPath = getSourceFile(Sel.AST->tuPath(), Sel); + + if (!OwnedPath) + return error("Couldn't find a suitable implementation file."); + Path = *OwnedPath; + assert(Sel.FS && "FS Must be set in apply"); + if (auto Buffer = Sel.FS->getBufferForFile(Path)) + OwnedBuffer = std::move(Buffer.get()); + else + // FIXME: Maybe we should consider creating the implementation file if + // it doesn't exist? + return llvm::errorCodeToError(Buffer.getError()); + TargetContents = OwnedBuffer->getBuffer(); + } else { + Path = Sel.AST->tuPath(); + TargetContents = Sel.Code; + } + + auto InsertionPoint = + getInsertionPoint(TargetContents, Source->getQualifiedNameAsString(), + Sel.AST->getLangOpts()); if (!InsertionPoint) return InsertionPoint.takeError(); @@ -452,14 +470,6 @@ if (!FuncDef) return FuncDef.takeError(); - SourceManagerForFile SMFF(*CCFile, Contents); - const tooling::Replacement InsertFunctionDef( - *CCFile, InsertionPoint->Offset, 0, *FuncDef); - auto Effect = Effect::mainFileEdit( - SMFF.get(), tooling::Replacements(InsertFunctionDef)); - if (!Effect) - return Effect.takeError(); - // FIXME: We should also get rid of inline qualifier. const tooling::Replacement DeleteFuncBody( Sel.AST->getSourceManager(), @@ -467,18 +477,35 @@ SM, Sel.AST->getLangOpts(), getDeletionRange(Source, Sel.AST->getTokens()))), ";"); - auto HeaderFE = Effect::fileEdit(SM, SM.getMainFileID(), - tooling::Replacements(DeleteFuncBody)); - if (!HeaderFE) - return HeaderFE.takeError(); - - Effect->ApplyEdits.try_emplace(HeaderFE->first, - std::move(HeaderFE->second)); - return std::move(*Effect); + const tooling::Replacement InsertFunctionDef(Path, InsertionPoint->Offset, + 0, *FuncDef); + + if (IsCrossFileEdit) { + SourceManagerForFile SMFF(Path, TargetContents); + auto Effect = Effect::mainFileEdit( + SMFF.get(), tooling::Replacements(InsertFunctionDef)); + if (!Effect) + return Effect.takeError(); + + auto HeaderFE = Effect::fileEdit(SM, SM.getMainFileID(), + tooling::Replacements(DeleteFuncBody)); + if (!HeaderFE) + return HeaderFE.takeError(); + + Effect->ApplyEdits.try_emplace(HeaderFE->first, + std::move(HeaderFE->second)); + return std::move(*Effect); + } + tooling::Replacements R(DeleteFuncBody); + if (auto Err = R.add(InsertFunctionDef)) { + return Err; + } + return Effect::mainFileEdit(SM, R); } private: const FunctionDecl *Source = nullptr; + bool IsCrossFileEdit = false; }; REGISTER_TWEAK(DefineOutline) diff --git a/clang-tools-extra/clangd/unittests/tweaks/DefineOutlineTests.cpp b/clang-tools-extra/clangd/unittests/tweaks/DefineOutlineTests.cpp --- a/clang-tools-extra/clangd/unittests/tweaks/DefineOutlineTests.cpp +++ b/clang-tools-extra/clangd/unittests/tweaks/DefineOutlineTests.cpp @@ -120,6 +120,22 @@ EXPECT_EQ(apply(Test), Expected); } +TEST_F(DefineOutlineTest, SourceFileAllowed) { + FileName = "Test.cpp"; + + llvm::StringRef Test = "struct A { A^() {} };\n"; + llvm::StringRef Expected = "struct A { A() ; };\nA::A() {} "; + EXPECT_EQ(Expected, apply(Test)); + + Test = "struct A { struct B { B^() {} }; };\n"; + Expected = "struct A { struct B { B() ; }; };\nA::B::B() {} "; + EXPECT_EQ(Expected, apply(Test)); + + Test = "struct A { struct B; };\nstruct A::B { B^() {} };\n"; + Expected = "struct A { struct B; };\nstruct A::B { B() ; };\nA::B::B() {} "; + EXPECT_EQ(Expected, apply(Test)); +} + TEST_F(DefineOutlineTest, ApplyTest) { llvm::StringMap EditedFiles; ExtraFiles["Test.cpp"] = "";