[clang-tools-extra] [clangd] Consider expression statements in ExtractVariable tweak (PR #112525)
Christian Kandeler via cfe-commits
cfe-commits at lists.llvm.org
Tue Dec 10 04:42:12 PST 2024
https://github.com/ckandeler updated https://github.com/llvm/llvm-project/pull/112525
>From 9993b1488c26b8296d3bb46b83e9c2f133d76c2d Mon Sep 17 00:00:00 2001
From: Christian Kandeler <christian.kandeler at qt.io>
Date: Tue, 23 May 2023 13:16:38 +0200
Subject: [PATCH] [clangd] Consider expression statements in ExtractVariable
tweak
For instance:
int func();
int main()
{
func(); // => auto placeholder = func();
}
---
.../refactor/tweaks/ExtractVariable.cpp | 49 +++++++++++++------
.../unittests/tweaks/ExtractVariableTests.cpp | 14 +++++-
2 files changed, 46 insertions(+), 17 deletions(-)
diff --git a/clang-tools-extra/clangd/refactor/tweaks/ExtractVariable.cpp b/clang-tools-extra/clangd/refactor/tweaks/ExtractVariable.cpp
index 3b378153eafd52..d84e501b87ce77 100644
--- a/clang-tools-extra/clangd/refactor/tweaks/ExtractVariable.cpp
+++ b/clang-tools-extra/clangd/refactor/tweaks/ExtractVariable.cpp
@@ -49,7 +49,8 @@ class ExtractionContext {
llvm::StringRef VarName) const;
// Generate Replacement for declaring the selected Expr as a new variable
tooling::Replacement insertDeclaration(llvm::StringRef VarName,
- SourceRange InitChars) const;
+ SourceRange InitChars,
+ bool AddSemicolon) const;
private:
bool Extractable = false;
@@ -252,7 +253,8 @@ ExtractionContext::replaceWithVar(SourceRange Chars,
// returns the Replacement for declaring a new variable storing the extraction
tooling::Replacement
ExtractionContext::insertDeclaration(llvm::StringRef VarName,
- SourceRange InitializerChars) const {
+ SourceRange InitializerChars,
+ bool AddSemicolon) const {
llvm::StringRef ExtractionCode = toSourceCode(SM, InitializerChars);
const SourceLocation InsertionLoc =
toHalfOpenFileRange(SM, Ctx.getLangOpts(),
@@ -260,7 +262,9 @@ ExtractionContext::insertDeclaration(llvm::StringRef VarName,
->getBegin();
std::string ExtractedVarDecl =
printType(VarType, ExprNode->getDeclContext(), VarName) + " = " +
- ExtractionCode.str() + "; ";
+ ExtractionCode.str();
+ if (AddSemicolon)
+ ExtractedVarDecl += "; ";
return tooling::Replacement(SM, InsertionLoc, 0, ExtractedVarDecl);
}
@@ -419,12 +423,10 @@ const SelectionTree::Node *getCallExpr(const SelectionTree::Node *DeclRef) {
// Returns true if Inner (which is a direct child of Outer) is appearing as
// a statement rather than an expression whose value can be used.
-bool childExprIsStmt(const Stmt *Outer, const Expr *Inner) {
+bool childExprIsDisallowedStmt(const Stmt *Outer, const Expr *Inner) {
if (!Outer || !Inner)
return false;
// Exclude the most common places where an expr can appear but be unused.
- if (llvm::isa<CompoundStmt>(Outer))
- return true;
if (llvm::isa<SwitchCase>(Outer))
return true;
// Control flow statements use condition etc, but not the body.
@@ -476,12 +478,9 @@ bool eligibleForExtraction(const SelectionTree::Node *N) {
const auto *Parent = OuterImplicit.Parent;
if (!Parent)
return false;
- // We don't want to extract expressions used as statements, that would leave
- // a `placeholder;` around that has no effect.
- // Unfortunately because the AST doesn't have ExprStmt, we have to check in
- // this roundabout way.
- if (childExprIsStmt(Parent->ASTNode.get<Stmt>(),
- OuterImplicit.ASTNode.get<Expr>()))
+ // Filter non-applicable expression statements.
+ if (childExprIsDisallowedStmt(Parent->ASTNode.get<Stmt>(),
+ OuterImplicit.ASTNode.get<Expr>()))
return false;
std::function<bool(const SelectionTree::Node *)> IsFullySelected =
@@ -516,6 +515,12 @@ bool eligibleForExtraction(const SelectionTree::Node *N) {
return false;
}
+ // If e.g. a capture clause was selected, the target node is the lambda
+ // expression. We only want to offer the extraction if the entire lambda
+ // expression was selected.
+ if (llvm::isa<LambdaExpr>(E))
+ return N->Selected == SelectionTree::Complete;
+
// The same logic as for assignments applies to initializations.
// However, we do allow extracting the RHS of an init capture, as it is
// a valid use case to move non-trivial expressions out of the capture clause.
@@ -599,10 +604,24 @@ Expected<Tweak::Effect> ExtractVariable::apply(const Selection &Inputs) {
// FIXME: get variable name from user or suggest based on type
std::string VarName = "placeholder";
SourceRange Range = Target->getExtractionChars();
- // insert new variable declaration
- if (auto Err = Result.add(Target->insertDeclaration(VarName, Range)))
+
+ const SelectionTree::Node &OuterImplicit =
+ Target->getExprNode()->outerImplicit();
+ assert(OuterImplicit.Parent);
+ bool IsExprStmt = llvm::isa_and_nonnull<CompoundStmt>(
+ OuterImplicit.Parent->ASTNode.get<Stmt>());
+
+ // insert new variable declaration. add a semicolon if and only if
+ // we are not dealing with an expression statement, which already has
+ // a semicolon that stays where it is, as it's not part of the range.
+ if (auto Err =
+ Result.add(Target->insertDeclaration(VarName, Range, !IsExprStmt)))
return std::move(Err);
- // replace expression with variable name
+
+ // replace expression with variable name, unless it's an expression statement,
+ // in which case we remove it.
+ if (IsExprStmt)
+ VarName.clear();
if (auto Err = Result.add(Target->replaceWithVar(Range, VarName)))
return std::move(Err);
return Effect::mainFileEdit(Inputs.AST->getSourceManager(),
diff --git a/clang-tools-extra/clangd/unittests/tweaks/ExtractVariableTests.cpp b/clang-tools-extra/clangd/unittests/tweaks/ExtractVariableTests.cpp
index 656b62c9a1f4e1..552e693c0363a8 100644
--- a/clang-tools-extra/clangd/unittests/tweaks/ExtractVariableTests.cpp
+++ b/clang-tools-extra/clangd/unittests/tweaks/ExtractVariableTests.cpp
@@ -151,8 +151,8 @@ TEST_F(ExtractVariableTest, Test) {
// Variable DeclRefExpr
a = [[b]];
a = [[xyz()]];
- // statement expression
- [[xyz()]];
+ // expression statement of type void
+ [[v()]];
while (a)
[[++a]];
// label statement
@@ -493,6 +493,16 @@ TEST_F(ExtractVariableTest, Test) {
a = a + 1;
}
})cpp"},
+ {R"cpp(
+ int func() { return 0; }
+ int main() {
+ [[func()]];
+ })cpp",
+ R"cpp(
+ int func() { return 0; }
+ int main() {
+ auto placeholder = func();
+ })cpp"},
{R"cpp(
template <typename T>
auto call(T t) { return t(); }
More information about the cfe-commits
mailing list