Index: clang-tools-extra/clangd/refactor/tweaks/ExtractFunction.cpp =================================================================== --- clang-tools-extra/clangd/refactor/tweaks/ExtractFunction.cpp +++ clang-tools-extra/clangd/refactor/tweaks/ExtractFunction.cpp @@ -237,9 +237,6 @@ if (CurNode->ASTNode.get()) return nullptr; if (const FunctionDecl *Func = CurNode->ASTNode.get()) { - // FIXME: Support extraction from methods. - if (isa(Func)) - return nullptr; // FIXME: Support extraction from templated functions. if (Func->isTemplated()) return nullptr; @@ -344,8 +341,14 @@ std::vector Parameters; SourceRange BodyRange; SourceLocation InsertionPoint; + llvm::Optional DeclarationPoint; + llvm::Optional ParentContext; const DeclContext *EnclosingFuncContext; bool CallerReturnsValue = false; + bool Static = false; + bool Constexpr = false; + bool OutOfLine = false; + bool ContextConst = false; // Decides whether the extracted function body and the function call need a // semicolon after extraction. tooling::ExtractionSemicolonPolicy SemicolonPolicy; @@ -354,11 +357,16 @@ // Render the call for this function. std::string renderCall() const; // Render the definition for this function. + std::string renderInlineDefinition(const SourceManager &SM) const; std::string renderDefinition(const SourceManager &SM) const; + std::string renderDeclaration(const SourceManager &SM) const; private: std::string renderParametersForDefinition() const; std::string renderParametersForCall() const; + std::string renderAttributes() const; + std::string renderAttributesAfter() const; + std::string renderNamespaceAndClass() const; // Generate the function body. std::string getFuncBody(const SourceManager &SM) const; }; @@ -387,6 +395,41 @@ return Result; } +std::string NewFunction::renderAttributes() const { + std::string Attributes; + + if (Static) { + Attributes += "static "; + } + + if (Constexpr) { + Attributes += "constexpr "; + } + + return Attributes; +} + +std::string NewFunction::renderAttributesAfter() const { + std::string Attributes; + + if (ContextConst) { + Attributes += " const"; + } + + return Attributes; +} + +std::string NewFunction::renderNamespaceAndClass() const { + std::string NamespaceClass; + + if (ParentContext) { + NamespaceClass = ParentContext.getValue()->getNameAsString(); + NamespaceClass += "::"; + } + + return NamespaceClass; +} + std::string NewFunction::renderCall() const { return std::string( llvm::formatv("{0}{1}({2}){3}", CallerReturnsValue ? "return " : "", Name, @@ -394,10 +437,24 @@ (SemicolonPolicy.isNeededInOriginalFunction() ? ";" : ""))); } +std::string NewFunction::renderInlineDefinition(const SourceManager &SM) const { + return std::string( + llvm::formatv("{0} {\n{1}\n}\n", renderDeclaration(SM), getFuncBody(SM))); +} + std::string NewFunction::renderDefinition(const SourceManager &SM) const { - return std::string(llvm::formatv( - "{0} {1}({2}) {\n{3}\n}\n", printType(ReturnType, *EnclosingFuncContext), - Name, renderParametersForDefinition(), getFuncBody(SM))); + return std::string(llvm::formatv("{0} {1}{2}({3}){4} {\n{5}\n}\n", + printType(ReturnType, *EnclosingFuncContext), + renderNamespaceAndClass(), Name, + renderParametersForDefinition(), + renderAttributesAfter(), getFuncBody(SM))); +} + +std::string NewFunction::renderDeclaration(const SourceManager &SM) const { + return std::string(llvm::formatv("{0}{1} {2}({3}){4}", renderAttributes(), + printType(ReturnType, *EnclosingFuncContext), + Name, renderParametersForDefinition(), + renderAttributesAfter())); } std::string NewFunction::getFuncBody(const SourceManager &SM) const { @@ -669,6 +726,24 @@ return error("Cannot extract break/continue without corresponding " "loop/switch statement."); NewFunction ExtractedFunc(getSemicolonPolicy(ExtZone, SM, LangOpts)); + + if (isa(ExtZone.EnclosingFunction)) { + const auto *Method = + llvm::dyn_cast(ExtZone.EnclosingFunction); + const auto *FirstOriginalDecl = Method->getCanonicalDecl(); + + ExtractedFunc.OutOfLine = Method->isOutOfLine(); + ExtractedFunc.Static = Method->isStatic(); + ExtractedFunc.Constexpr = Method->isConstexpr(); + ExtractedFunc.ContextConst = Method->isConst(); + + auto DeclPos = + toHalfOpenFileRange(SM, LangOpts, FirstOriginalDecl->getSourceRange()); + + ExtractedFunc.ParentContext = Method->getParent(); + ExtractedFunc.DeclarationPoint = DeclPos.getValue(); + } + ExtractedFunc.BodyRange = ExtZone.ZoneRange; ExtractedFunc.InsertionPoint = ExtZone.getInsertionPoint(); ExtractedFunc.EnclosingFuncContext = @@ -706,10 +781,42 @@ tooling::Replacement createFunctionDefinition(const NewFunction &ExtractedFunc, const SourceManager &SM) { - std::string FunctionDef = ExtractedFunc.renderDefinition(SM); + std::string FunctionDef; + + if (ExtractedFunc.OutOfLine) { + FunctionDef = ExtractedFunc.renderDefinition(SM); + } else { + FunctionDef = ExtractedFunc.renderInlineDefinition(SM); + } + return tooling::Replacement(SM, ExtractedFunc.InsertionPoint, 0, FunctionDef); } +tooling::Replacement createFunctionDeclaration(const NewFunction &ExtractedFunc, + const SourceManager &SM) { + auto FunctionDecl = ExtractedFunc.renderDeclaration(SM) + ";\n"; + auto DeclPoint = ExtractedFunc.DeclarationPoint.getValue(); + + return tooling::Replacement(SM, DeclPoint.getBegin(), 0, FunctionDecl); +} + +llvm::Error addDeclarationTweakEffect(llvm::Expected &FileEdit, + const NewFunction &ExtractedFunc, + const SourceManager &SM) { + auto DeclPoint = ExtractedFunc.DeclarationPoint.getValue(); + tooling::Replacements ResultDecl; + if (auto Err = ResultDecl.add(createFunctionDeclaration(ExtractedFunc, SM))) + return Err; + auto PathAndEdit = Tweak::Effect::fileEdit( + SM, SM.getFileID(DeclPoint.getBegin()), std::move(ResultDecl)); + if (!PathAndEdit) + return PathAndEdit.takeError(); + + FileEdit->ApplyEdits.try_emplace(PathAndEdit->first, PathAndEdit->second); + + return llvm::Error::success(); +} + // Returns true if ExtZone contains any ReturnStmts. bool hasReturnStmt(const ExtractionZone &ExtZone) { class ReturnStmtVisitor @@ -762,7 +869,24 @@ return std::move(Err); if (auto Err = Result.add(replaceWithFuncCall(*ExtractedFunc, SM, LangOpts))) return std::move(Err); - return Effect::mainFileEdit(SM, std::move(Result)); + + auto DefinitionIsOutOfLine = + ExtractedFunc->OutOfLine && ExtractedFunc->DeclarationPoint.hasValue(); + + if (!DefinitionIsOutOfLine) { + return Effect::mainFileEdit(SM, std::move(Result)); + } + + if (Result.add(createFunctionDeclaration(*ExtractedFunc, SM)).success()) { + return Effect::mainFileEdit(SM, std::move(Result)); + } + + auto MainRes = Effect::mainFileEdit(SM, std::move(Result)); + if (MainRes) { + if (auto Err = addDeclarationTweakEffect(MainRes, *ExtractedFunc, SM)) + return std::move(Err); + } + return MainRes; } } // namespace Index: clang-tools-extra/clangd/unittests/tweaks/ExtractFunctionTests.cpp =================================================================== --- clang-tools-extra/clangd/unittests/tweaks/ExtractFunctionTests.cpp +++ clang-tools-extra/clangd/unittests/tweaks/ExtractFunctionTests.cpp @@ -114,16 +114,48 @@ // Don't extract when we need to make a function as a parameter. EXPECT_THAT(apply("void f() { [[int a; f();]] }"), StartsWith("fail")); - // We don't extract from methods for now since they may involve multi-file - // edits - std::string MethodFailInput = R"cpp( + std::string MethodInput = R"cpp( class T { void f() { [[int x;]] } }; )cpp"; - EXPECT_EQ(apply(MethodFailInput), "unavailable"); + std::string MethodCheckOutput = R"cpp( + class T { + void extracted() { +int x; +} +void f() { + extracted(); + } + }; + )cpp"; + EXPECT_EQ(apply(MethodInput), MethodCheckOutput); + + std::string OutOfLineMethodInput = R"cpp( + class T { + void f(); + }; + + void T::f() { + [[int x;]] + } + )cpp"; + std::string OutOfLineMethodCheckOutput = R"cpp( + class T { + void extracted(); +void f(); + }; + + void T::extracted() { +int x; +} +void T::f() { + extracted(); + } + )cpp"; + EXPECT_EQ(apply(OutOfLineMethodInput), OutOfLineMethodCheckOutput); // We don't extract from templated functions for now as templates are hard // to deal with. @@ -159,6 +191,117 @@ EXPECT_EQ(apply(CompoundFailInput), "unavailable"); } +TEST_F(ExtractFunctionTest, DifferentHeaderSourceTest) { + Header = R"cpp( + class SomeClass { + void f(); + }; + )cpp"; + + std::string OutOfLineSource = R"cpp( + void SomeClass::f() { + [[int x;]] + } + )cpp"; + + std::string OutOfLineSourceOutputCheck = R"cpp( + void SomeClass::extracted() { +int x; +} +void SomeClass::f() { + extracted(); + } + )cpp"; + + std::string HeaderOutputCheck = R"cpp( + class SomeClass { + void extracted(); +void f(); + }; + )cpp"; + + llvm::StringMap EditedFiles; + + EXPECT_EQ(apply(OutOfLineSource, &EditedFiles), OutOfLineSourceOutputCheck); + EXPECT_EQ(EditedFiles.begin()->second, HeaderOutputCheck); +} + +TEST_F(ExtractFunctionTest, ConstDifferentHeaderSourceTest) { + Header = R"cpp( + class SomeClass { + void f() const; + }; + )cpp"; + + std::string OutOfLineSource = R"cpp( + void SomeClass::f() const { + [[int x;]] + } + )cpp"; + + std::string OutOfLineSourceOutputCheck = R"cpp( + void SomeClass::extracted() const { +int x; +} +void SomeClass::f() const { + extracted(); + } + )cpp"; + + std::string HeaderOutputCheck = R"cpp( + class SomeClass { + void extracted() const; +void f() const; + }; + )cpp"; + + llvm::StringMap EditedFiles; + + EXPECT_EQ(apply(OutOfLineSource, &EditedFiles), OutOfLineSourceOutputCheck); + EXPECT_NE(EditedFiles.begin(), EditedFiles.end()) + << "The header should be edited and receives the declaration of the new " + "function"; + + if (EditedFiles.begin() != EditedFiles.end()) { + EXPECT_EQ(EditedFiles.begin()->second, HeaderOutputCheck); + } +} + +TEST_F(ExtractFunctionTest, StaticDifferentHeaderSourceTest) { + Header = R"cpp( + class SomeClass { + static void f(); + }; + )cpp"; + + std::string OutOfLineSource = R"cpp( + void SomeClass::f() { + [[int x;]] + } + )cpp"; + + std::string OutOfLineSourceOutputCheck = R"cpp( + void SomeClass::extracted() { +int x; +} +void SomeClass::f() { + extracted(); + } + )cpp"; + + std::string HeaderOutputCheck = R"cpp( + class SomeClass { + static void extracted(); +static void f(); + }; + )cpp"; + + llvm::StringMap EditedFiles; + + EXPECT_EQ(apply(OutOfLineSource, &EditedFiles), OutOfLineSourceOutputCheck); + EXPECT_EQ(EditedFiles.begin()->second, HeaderOutputCheck); +} + TEST_F(ExtractFunctionTest, ControlFlow) { Context = Function; // We should be able to extract break/continue with a parent loop/switch.