[clang-tools-extra] 8d714db - [clangd] Consider expression statements in ExtractVariable tweak (#112525)
via cfe-commits
cfe-commits at lists.llvm.org
Wed Dec 11 01:58:26 PST 2024
Author: Christian Kandeler
Date: 2024-12-11T10:58:22+01:00
New Revision: 8d714db7f9617252401f85537d672c5b92c20557
URL: https://github.com/llvm/llvm-project/commit/8d714db7f9617252401f85537d672c5b92c20557
DIFF: https://github.com/llvm/llvm-project/commit/8d714db7f9617252401f85537d672c5b92c20557.diff
LOG: [clangd] Consider expression statements in ExtractVariable tweak (#112525)
For instance:
int func();
int main()
{
func(); // => auto placeholder = func();
}
Added:
Modified:
clang-tools-extra/clangd/refactor/tweaks/ExtractVariable.cpp
clang-tools-extra/clangd/unittests/tweaks/ExtractVariableTests.cpp
Removed:
################################################################################
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