[clang-tools-extra] [clangd] Allow "move function body out-of-line" in non-header files (PR #69704)
Christian Kandeler via cfe-commits
cfe-commits at lists.llvm.org
Tue Nov 21 09:26:26 PST 2023
https://github.com/ckandeler updated https://github.com/llvm/llvm-project/pull/69704
>From 40df0527b2a3af8012f32d771a1bb2c861d42ed3 Mon Sep 17 00:00:00 2001
From: Christian Kandeler <christian.kandeler at qt.io>
Date: Thu, 19 Oct 2023 17:51:11 +0200
Subject: [PATCH] [clangd] Allow "move function body out-of-line" in non-header
files
Moving the body of member functions out-of-line makes sense for classes
defined in implementation files too.
---
.../clangd/refactor/tweaks/DefineOutline.cpp | 150 +++++++++++-------
.../unittests/tweaks/DefineOutlineTests.cpp | 73 ++++++++-
2 files changed, 164 insertions(+), 59 deletions(-)
diff --git a/clang-tools-extra/clangd/refactor/tweaks/DefineOutline.cpp b/clang-tools-extra/clangd/refactor/tweaks/DefineOutline.cpp
index b84ae04072f2c19..98cb3a8770c696d 100644
--- a/clang-tools-extra/clangd/refactor/tweaks/DefineOutline.cpp
+++ b/clang-tools-extra/clangd/refactor/tweaks/DefineOutline.cpp
@@ -179,14 +179,11 @@ deleteTokensWithKind(const syntax::TokenBuffer &TokBuf, tok::TokenKind Kind,
// looked up in the context containing the function/method.
// FIXME: Drop attributes in function signature.
llvm::Expected<std::string>
-getFunctionSourceCode(const FunctionDecl *FD, llvm::StringRef TargetNamespace,
+getFunctionSourceCode(const FunctionDecl *FD, const DeclContext *TargetContext,
const syntax::TokenBuffer &TokBuf,
const HeuristicResolver *Resolver) {
auto &AST = FD->getASTContext();
auto &SM = AST.getSourceManager();
- auto TargetContext = findContextForNS(TargetNamespace, FD->getDeclContext());
- if (!TargetContext)
- return error("define outline: couldn't find a context for target");
llvm::Error Errors = llvm::Error::success();
tooling::Replacements DeclarationCleanups;
@@ -216,7 +213,7 @@ getFunctionSourceCode(const FunctionDecl *FD, llvm::StringRef TargetNamespace,
}
const NamedDecl *ND = Ref.Targets.front();
const std::string Qualifier =
- getQualification(AST, *TargetContext,
+ getQualification(AST, TargetContext,
SM.getLocForStartOfFile(SM.getMainFileID()), ND);
if (auto Err = DeclarationCleanups.add(
tooling::Replacement(SM, Ref.NameLoc, 0, Qualifier)))
@@ -232,7 +229,7 @@ getFunctionSourceCode(const FunctionDecl *FD, llvm::StringRef TargetNamespace,
if (const auto *Destructor = llvm::dyn_cast<CXXDestructorDecl>(FD)) {
if (auto Err = DeclarationCleanups.add(tooling::Replacement(
SM, Destructor->getLocation(), 0,
- getQualification(AST, *TargetContext,
+ getQualification(AST, TargetContext,
SM.getLocForStartOfFile(SM.getMainFileID()),
Destructor))))
Errors = llvm::joinErrors(std::move(Errors), std::move(Err));
@@ -319,29 +316,9 @@ getFunctionSourceCode(const FunctionDecl *FD, llvm::StringRef TargetNamespace,
}
struct InsertionPoint {
- std::string EnclosingNamespace;
+ const DeclContext *EnclosingNamespace = nullptr;
size_t Offset;
};
-// Returns the most natural insertion point for \p QualifiedName in \p Contents.
-// This currently cares about only the namespace proximity, but in feature it
-// should also try to follow ordering of declarations. For example, if decls
-// come in order `foo, bar, baz` then this function should return some point
-// between foo and baz for inserting bar.
-llvm::Expected<InsertionPoint> getInsertionPoint(llvm::StringRef Contents,
- llvm::StringRef QualifiedName,
- const LangOptions &LangOpts) {
- auto Region = getEligiblePoints(Contents, QualifiedName, LangOpts);
-
- assert(!Region.EligiblePoints.empty());
- // FIXME: This selection can be made smarter by looking at the definition
- // locations for adjacent decls to Source. Unfortunately pseudo parsing in
- // getEligibleRegions only knows about namespace begin/end events so we
- // can't match function start/end positions yet.
- auto Offset = positionToOffset(Contents, Region.EligiblePoints.back());
- if (!Offset)
- return Offset.takeError();
- return InsertionPoint{Region.EnclosingNamespace, *Offset};
-}
// Returns the range that should be deleted from declaration, which always
// contains function body. In addition to that it might contain constructor
@@ -409,14 +386,9 @@ class DefineOutline : public Tweak {
}
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;
-
+ SameFile = !isHeaderFile(Sel.AST->tuPath(), Sel.AST->getLangOpts());
Source = getSelectedFunction(Sel.ASTSelection.commonAncestor());
+
// Bail out if the selection is not a in-line function definition.
if (!Source || !Source->doesThisDeclarationHaveABody() ||
Source->isOutOfLine())
@@ -429,19 +401,24 @@ class DefineOutline : public Tweak {
if (Source->getTemplateSpecializationInfo())
return false;
- if (auto *MD = llvm::dyn_cast<CXXMethodDecl>(Source)) {
- // Bail out in templated classes, as it is hard to spell the class name,
- // i.e if the template parameter is unnamed.
- if (MD->getParent()->isTemplated())
- return false;
-
- // The refactoring is meaningless for unnamed classes and definitions
- // within unnamed namespaces.
- for (const DeclContext *DC = MD->getParent(); DC; DC = DC->getParent()) {
- if (auto *ND = llvm::dyn_cast<NamedDecl>(DC)) {
- if (ND->getDeclName().isEmpty())
- return false;
- }
+ auto *MD = llvm::dyn_cast<CXXMethodDecl>(Source);
+ if (!MD) {
+ // Can't outline free-standing functions in the same file.
+ return !SameFile;
+ }
+
+ // Bail out in templated classes, as it is hard to spell the class name,
+ // i.e if the template parameter is unnamed.
+ if (MD->getParent()->isTemplated())
+ return false;
+
+ // The refactoring is meaningless for unnamed classes and namespaces,
+ // unless we're outlining in the same file
+ for (const DeclContext *DC = MD->getParent(); DC; DC = DC->getParent()) {
+ if (auto *ND = llvm::dyn_cast<NamedDecl>(DC)) {
+ if (ND->getDeclName().isEmpty() &&
+ (!SameFile || !llvm::dyn_cast<NamespaceDecl>(ND)))
+ return false;
}
}
@@ -453,8 +430,8 @@ class DefineOutline : public Tweak {
Expected<Effect> apply(const Selection &Sel) override {
const SourceManager &SM = Sel.AST->getSourceManager();
- auto CCFile = getSourceFile(Sel.AST->tuPath(), Sel);
-
+ auto CCFile = SameFile ? Sel.AST->tuPath().str()
+ : 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");
@@ -464,8 +441,7 @@ class DefineOutline : public Tweak {
if (!Buffer)
return llvm::errorCodeToError(Buffer.getError());
auto Contents = Buffer->get()->getBuffer();
- auto InsertionPoint = getInsertionPoint(
- Contents, Source->getQualifiedNameAsString(), Sel.AST->getLangOpts());
+ auto InsertionPoint = getInsertionPoint(Contents, Sel);
if (!InsertionPoint)
return InsertionPoint.takeError();
@@ -499,17 +475,77 @@ class DefineOutline : public Tweak {
HeaderUpdates = HeaderUpdates.merge(*DelInline);
}
- auto HeaderFE = Effect::fileEdit(SM, SM.getMainFileID(), HeaderUpdates);
- if (!HeaderFE)
- return HeaderFE.takeError();
-
- Effect->ApplyEdits.try_emplace(HeaderFE->first,
- std::move(HeaderFE->second));
+ if (SameFile) {
+ tooling::Replacements &R = Effect->ApplyEdits[*CCFile].Replacements;
+ R = R.merge(HeaderUpdates);
+ } else {
+ auto HeaderFE = Effect::fileEdit(SM, SM.getMainFileID(), HeaderUpdates);
+ if (!HeaderFE)
+ return HeaderFE.takeError();
+ Effect->ApplyEdits.try_emplace(HeaderFE->first,
+ std::move(HeaderFE->second));
+ }
return std::move(*Effect);
}
+ // Returns the most natural insertion point for \p QualifiedName in \p
+ // Contents. This currently cares about only the namespace proximity, but in
+ // feature it should also try to follow ordering of declarations. For example,
+ // if decls come in order `foo, bar, baz` then this function should return
+ // some point between foo and baz for inserting bar.
+ // FIXME: The selection can be made smarter by looking at the definition
+ // locations for adjacent decls to Source. Unfortunately pseudo parsing in
+ // getEligibleRegions only knows about namespace begin/end events so we
+ // can't match function start/end positions yet.
+ llvm::Expected<InsertionPoint> getInsertionPoint(llvm::StringRef Contents,
+ const Selection &Sel) {
+ // If the definition goes to the same file and there is a namespace,
+ // we should (and, in the case of anonymous namespaces, need to)
+ // put the definition into the original namespace block.
+ if (SameFile) {
+ auto *Klass = Source->getDeclContext()->getOuterLexicalRecordContext();
+ if (!Klass)
+ return error("moving to same file not supported for free functions");
+ const SourceLocation EndLoc = Klass->getBraceRange().getEnd();
+ const auto &TokBuf = Sel.AST->getTokens();
+ auto Tokens = TokBuf.expandedTokens();
+ auto It = llvm::lower_bound(
+ Tokens, EndLoc, [](const syntax::Token &Tok, SourceLocation EndLoc) {
+ return Tok.location() < EndLoc;
+ });
+ while (It != Tokens.end()) {
+ if (It->kind() != tok::semi) {
+ ++It;
+ continue;
+ }
+ unsigned Offset = Sel.AST->getSourceManager()
+ .getDecomposedLoc(It->endLocation())
+ .second;
+ return InsertionPoint{Klass->getEnclosingNamespaceContext(), Offset};
+ }
+ return error(
+ "failed to determine insertion location: no end of class found");
+ }
+
+ auto Region = getEligiblePoints(
+ Contents, Source->getQualifiedNameAsString(), Sel.AST->getLangOpts());
+
+ assert(!Region.EligiblePoints.empty());
+ auto Offset = positionToOffset(Contents, Region.EligiblePoints.back());
+ if (!Offset)
+ return Offset.takeError();
+
+ auto TargetContext =
+ findContextForNS(Region.EnclosingNamespace, Source->getDeclContext());
+ if (!TargetContext)
+ return error("define outline: couldn't find a context for target");
+
+ return InsertionPoint{*TargetContext, *Offset};
+ }
+
private:
const FunctionDecl *Source = nullptr;
+ bool SameFile = false;
};
REGISTER_TWEAK(DefineOutline)
diff --git a/clang-tools-extra/clangd/unittests/tweaks/DefineOutlineTests.cpp b/clang-tools-extra/clangd/unittests/tweaks/DefineOutlineTests.cpp
index d1e60b070f20e95..906ff33db873441 100644
--- a/clang-tools-extra/clangd/unittests/tweaks/DefineOutlineTests.cpp
+++ b/clang-tools-extra/clangd/unittests/tweaks/DefineOutlineTests.cpp
@@ -19,12 +19,47 @@ TWEAK_TEST(DefineOutline);
TEST_F(DefineOutlineTest, TriggersOnFunctionDecl) {
FileName = "Test.cpp";
- // Not available unless in a header file.
+ // Not available for free function unless in a header file.
EXPECT_UNAVAILABLE(R"cpp(
[[void [[f^o^o]]() [[{
return;
}]]]])cpp");
+ // Available in soure file.
+ EXPECT_AVAILABLE(R"cpp(
+ struct Foo {
+ void f^oo() {}
+ };
+ )cpp");
+
+ // Available within named namespace in source file.
+ EXPECT_AVAILABLE(R"cpp(
+ namespace N {
+ struct Foo {
+ void f^oo() {}
+ };
+ } // namespace N
+ )cpp");
+
+ // Available within anonymous namespace in source file.
+ EXPECT_AVAILABLE(R"cpp(
+ namespace {
+ struct Foo {
+ void f^oo() {}
+ };
+ } // namespace
+ )cpp");
+
+ // Not available for out-of-line method.
+ EXPECT_UNAVAILABLE(R"cpp(
+ class Bar {
+ void baz();
+ };
+
+ [[void [[Bar::[[b^a^z]]]]() [[{
+ return;
+ }]]]])cpp");
+
FileName = "Test.hpp";
// Not available unless function name or fully body is selected.
EXPECT_UNAVAILABLE(R"cpp(
@@ -100,7 +135,7 @@ TEST_F(DefineOutlineTest, TriggersOnFunctionDecl) {
};
)cpp");
- // Not available on definitions within unnamed namespaces
+ // Not available on definitions in header file within unnamed namespaces
EXPECT_UNAVAILABLE(R"cpp(
namespace {
struct Foo {
@@ -349,6 +384,40 @@ TEST_F(DefineOutlineTest, ApplyTest) {
}
}
+TEST_F(DefineOutlineTest, InCppFile) {
+ FileName = "Test.cpp";
+
+ struct {
+ llvm::StringRef Test;
+ llvm::StringRef ExpectedSource;
+ } Cases[] = {
+ {
+ R"cpp(
+ namespace foo {
+ namespace {
+ struct Foo { void ba^r() {} };
+ struct Bar { void foo(); };
+ void Bar::foo() {}
+ }
+ }
+ )cpp",
+ R"cpp(
+ namespace foo {
+ namespace {
+ struct Foo { void bar() ; };void Foo::bar() {}
+ struct Bar { void foo(); };
+ void Bar::foo() {}
+ }
+ }
+ )cpp"},
+ };
+
+ for (const auto &Case : Cases) {
+ SCOPED_TRACE(Case.Test);
+ EXPECT_EQ(apply(Case.Test, nullptr), Case.ExpectedSource);
+ }
+}
+
TEST_F(DefineOutlineTest, HandleMacros) {
llvm::StringMap<std::string> EditedFiles;
ExtraFiles["Test.cpp"] = "";
More information about the cfe-commits
mailing list