Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 11 additions & 8 deletions clang-tools-extra/clangd/refactor/tweaks/ExtractFunction.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,7 @@
#include "clang/AST/ASTContext.h"
#include "clang/AST/Decl.h"
#include "clang/AST/DeclBase.h"
#include "clang/AST/ExprCXX.h"
#include "clang/AST/NestedNameSpecifier.h"
#include "clang/AST/RecursiveASTVisitor.h"
#include "clang/AST/Stmt.h"
Expand All @@ -70,7 +71,6 @@
#include "llvm/ADT/StringRef.h"
#include "llvm/Support/Casting.h"
#include "llvm/Support/Error.h"
#include "llvm/Support/raw_os_ostream.h"
#include <optional>

namespace clang {
Expand All @@ -95,18 +95,21 @@ enum FunctionDeclKind {
OutOfLineDefinition
};

// A RootStmt is a statement that's fully selected including all it's children
// and it's parent is unselected.
// A RootStmt is a statement that's fully selected including all its children
// and its parent is unselected.
// Check if a node is a root statement.
bool isRootStmt(const Node *N) {
if (!N->ASTNode.get<Stmt>())
return false;
// Root statement cannot be partially selected.
if (N->Selected == SelectionTree::Partial)
return false;
// Only DeclStmt can be an unselected RootStmt since VarDecls claim the entire
// selection range in selectionTree.
if (N->Selected == SelectionTree::Unselected && !N->ASTNode.get<DeclStmt>())
// A DeclStmt can be an unselected RootStmt since VarDecls claim the entire
// selection range in selectionTree. Additionally, a CXXOperatorCallExpr of a
// binary operation can be unselected because its children claim the entire
// selection range in the selection tree (e.g. <<).
if (N->Selected == SelectionTree::Unselected && !N->ASTNode.get<DeclStmt>() &&
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need to make a corresponding change here? The comment suggests that it's special-casing DeclStmt for the same reason as this code.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think that would have any effect, because to hit this case, the common ancestor of our root statements needs to be completely selected, but the CXXOperatorCallExpr will be unselected as shown in this if here.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess the difference is that a DeclStmt can have a single VarDecl child which could be completely selected, leaving the DeclStmt unselected. There is no analogous situation for a CXXOperatorCallExpr which has multiple children.

!N->ASTNode.get<CXXOperatorCallExpr>())
return false;
return true;
}
Expand Down Expand Up @@ -913,8 +916,8 @@ Expected<Tweak::Effect> ExtractFunction::apply(const Selection &Inputs) {

tooling::Replacements OtherEdit(
createForwardDeclaration(*ExtractedFunc, SM));
if (auto PathAndEdit = Tweak::Effect::fileEdit(SM, SM.getFileID(*FwdLoc),
OtherEdit))
if (auto PathAndEdit =
Tweak::Effect::fileEdit(SM, SM.getFileID(*FwdLoc), OtherEdit))
MultiFileEffect->ApplyEdits.try_emplace(PathAndEdit->first,
PathAndEdit->second);
else
Expand Down
59 changes: 59 additions & 0 deletions clang-tools-extra/clangd/unittests/tweaks/ExtractFunctionTests.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -190,6 +190,18 @@ F (extracted();)
}]]
)cpp";
EXPECT_EQ(apply(CompoundFailInput), "unavailable");

ExtraArgs.push_back("-std=c++14");
// FIXME: Expressions are currently not extracted
EXPECT_EQ(apply(R"cpp(
void call() { [[1+1]]; }
)cpp"),
"unavailable");
// FIXME: Single expression statements are currently not extracted
EXPECT_EQ(apply(R"cpp(
void call() { [[1+1;]] }
)cpp"),
"unavailable");
}

TEST_F(ExtractFunctionTest, DifferentHeaderSourceTest) {
Expand Down Expand Up @@ -571,6 +583,53 @@ int getNum(bool Superstitious, int Min, int Max) {
EXPECT_EQ(apply(Before), After);
}

TEST_F(ExtractFunctionTest, OverloadedOperators) {
Context = File;
std::string Before = R"cpp(struct A {
int operator+(int x) { return x; }
};
A &operator<<(A &, int);
A &operator|(A &, int);

A stream{};

void foo(int, int);

int main() {
[[foo(1, 2);
foo(3, 4);
stream << 42;
stream + 42;
stream | 42;
foo(1, 2);
foo(3, 4);]]
})cpp";
std::string After =
R"cpp(struct A {
int operator+(int x) { return x; }
};
A &operator<<(A &, int);
A &operator|(A &, int);

A stream{};

void foo(int, int);

void extracted() {
foo(1, 2);
foo(3, 4);
stream << 42;
stream + 42;
stream | 42;
foo(1, 2);
foo(3, 4);
}
int main() {
extracted();
})cpp";
EXPECT_EQ(apply(Before), After);
}

} // namespace
} // namespace clangd
} // namespace clang
3 changes: 3 additions & 0 deletions clang-tools-extra/docs/ReleaseNotes.rst
Original file line number Diff line number Diff line change
Expand Up @@ -78,6 +78,9 @@ Code actions

- Added `Swap operands` tweak for certain binary operators.

- Improved the extract-to-function code action to allow extracting statements
with overloaded operators like ``<<`` of ``std::ostream``.

Signature help
^^^^^^^^^^^^^^

Expand Down
Loading