https://github.com/ckandeler updated
https://github.com/llvm/llvm-project/pull/69704
>From b88cdbcd106e27d3e594dc06824df10d77f9402b Mon Sep 17 00:00:00 2001
From: Christian Kandeler <christian.kande...@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 | 115 +++++++++++-------
.../unittests/tweaks/DefineOutlineTests.cpp | 40 +++++-
2 files changed, 107 insertions(+), 48 deletions(-)
diff --git a/clang-tools-extra/clangd/refactor/tweaks/DefineOutline.cpp
b/clang-tools-extra/clangd/refactor/tweaks/DefineOutline.cpp
index b84ae04072f2c19..7a64cf6cd006fd6 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())
@@ -435,14 +407,17 @@ class DefineOutline : public Tweak {
if (MD->getParent()->isTemplated())
return false;
- // The refactoring is meaningless for unnamed classes and definitions
- // within unnamed namespaces.
+ // The refactoring is meaningless for unnamed classes.
for (const DeclContext *DC = MD->getParent(); DC; DC = DC->getParent()) {
if (auto *ND = llvm::dyn_cast<NamedDecl>(DC)) {
- if (ND->getDeclName().isEmpty())
+ if (ND->getDeclName().isEmpty() &&
+ (!SameFile || !llvm::dyn_cast<NamespaceDecl>(ND)))
return false;
}
}
+ } else if (SameFile) {
+ // Bail out for free-standing functions in non-header file.
+ return false;
}
// Note that we don't check whether an implementation file exists or not in
@@ -453,8 +428,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 +439,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 +473,64 @@ 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.
+ 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.
+ // Within this constraint, the same considerations apply as in
+ // the FIXME below.
+ if (SameFile) {
+ if (auto *Namespace = Source->getEnclosingNamespaceContext()) {
+ const SourceLocation EndLoc =
+ llvm::cast<NamespaceDecl>(Namespace)->getRBraceLoc();
+ unsigned Offset =
+
Sel.AST->getSourceManager().getDecomposedSpellingLoc(EndLoc).second;
+ return InsertionPoint{Namespace, Offset};
+ }
+ }
+
+ auto Region = getEligiblePoints(
+ Contents, Source->getQualifiedNameAsString(), Sel.AST->getLangOpts());
+
+ 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();
+
+ 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..cc04b6c16075555 100644
--- a/clang-tools-extra/clangd/unittests/tweaks/DefineOutlineTests.cpp
+++ b/clang-tools-extra/clangd/unittests/tweaks/DefineOutlineTests.cpp
@@ -19,7 +19,7 @@ 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;
@@ -349,6 +349,44 @@ TEST_F(DefineOutlineTest, ApplyTest) {
}
}
+TEST_F(DefineOutlineTest, InCppFile) {
+ FileName = "Test.cpp";
+
+ struct {
+ llvm::StringRef Test;
+ llvm::StringRef ExpectedSource;
+ } Cases[] = {
+ // Member function with some adornments
+ // FIXME: What's with the extra spaces?
+ {
+ "namespace {\n"
+ "enum class E { V1, V2 };\n"
+ "#define OVERRIDE override\n"
+ "struct Base { virtual E func() const = 0; };\n"
+ "struct Derived : Base {\n"
+ " inline E f^unc() const OVERRIDE { return E::V1; }\n"
+ "};\n"
+ "} // namespace\n"
+ "int main() {}",
+ "namespace {\n"
+ "enum class E { V1, V2 };\n"
+ "#define OVERRIDE override\n"
+ "struct Base { virtual E func() const = 0; };\n"
+ "struct Derived : Base {\n"
+ " E func() const OVERRIDE ;\n"
+ "};\n"
+ " E Derived::func() const { return E::V1; }\n"
+ "} // namespace\n"
+ "int main() {}",
+ },
+ };
+
+ 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"] = "";
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
