FabioRS created this revision.
FabioRS added a reviewer: sammccall.
FabioRS added a project: clang-tools-extra.
Herald added subscribers: usaxena95, kadircet, arphaman.
Herald added a project: All.
FabioRS requested review of this revision.
Herald added subscribers: cfe-commits, MaskRay, ilya-biryukov.
I miss more automatically refactoring functions when working with already
running code, so I am making some small addition that I hope help more people.
This works by checking if the function is a method (CXXMethodDecl), then
collecting information about the function that the code is being extracted,
looking for the declaration if it is out-of-line, creating the declaration if
it is necessary and putting the extracted function as a class-method.
This is my first code review request, sorry if I did something wrong.
Repository:
rG LLVM Github Monorepo
https://reviews.llvm.org/D122698
Files:
clang-tools-extra/clangd/refactor/tweaks/ExtractFunction.cpp
clang-tools-extra/clangd/unittests/tweaks/ExtractFunctionTests.cpp
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<std::string> 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<std::string> 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<std::string> 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.
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<LambdaExpr>())
return nullptr;
if (const FunctionDecl *Func = CurNode->ASTNode.get<FunctionDecl>()) {
- // FIXME: Support extraction from methods.
- if (isa<CXXMethodDecl>(Func))
- return nullptr;
// FIXME: Support extraction from templated functions.
if (Func->isTemplated())
return nullptr;
@@ -344,8 +341,14 @@
std::vector<Parameter> Parameters;
SourceRange BodyRange;
SourceLocation InsertionPoint;
+ llvm::Optional<SourceRange> DeclarationPoint;
+ llvm::Optional<const CXXRecordDecl *> 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<CXXMethodDecl>(ExtZone.EnclosingFunction)) {
+ const auto *Method =
+ llvm::dyn_cast<CXXMethodDecl>(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<Tweak::Effect> &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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
