sammccall updated this revision to Diff 212432.
sammccall marked 2 inline comments as done.
sammccall added a comment.
Address review comments.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D65337/new/
https://reviews.llvm.org/D65337
Files:
clang-tools-extra/clangd/Selection.cpp
clang-tools-extra/clangd/Selection.h
clang-tools-extra/clangd/refactor/tweaks/ExtractVariable.cpp
clang-tools-extra/clangd/unittests/TweakTests.cpp
Index: clang-tools-extra/clangd/unittests/TweakTests.cpp
===================================================================
--- clang-tools-extra/clangd/unittests/TweakTests.cpp
+++ clang-tools-extra/clangd/unittests/TweakTests.cpp
@@ -326,7 +326,7 @@
a = [[2]];
// while
while(a < [[1]])
- [[a++]];
+ a = [[1]];
// do while
do
a = [[1]];
@@ -370,11 +370,14 @@
// lambda
auto lamb = [&[[a]], &[[b]]](int r = [[1]]) {return 1;}
// assigment
- [[a = 5]];
- [[a >>= 5]];
- [[a *= 5]];
+ xyz([[a = 5]]);
+ xyz([[a *= 5]]);
// Variable DeclRefExpr
a = [[b]];
+ // statement expression
+ [[xyz()]];
+ while (a)
+ [[++a]];
// label statement
goto label;
label:
@@ -417,11 +420,11 @@
// Macros
{R"cpp(#define PLUS(x) x++
void f(int a) {
- PLUS([[1+a]]);
+ int y = PLUS([[1+a]]);
})cpp",
R"cpp(#define PLUS(x) x++
void f(int a) {
- auto dummy = PLUS(1+a); dummy;
+ auto dummy = PLUS(1+a); int y = dummy;
})cpp"},
// ensure InsertionPoint isn't inside a macro
{R"cpp(#define LOOP(x) while (1) {a = x;}
Index: clang-tools-extra/clangd/refactor/tweaks/ExtractVariable.cpp
===================================================================
--- clang-tools-extra/clangd/refactor/tweaks/ExtractVariable.cpp
+++ clang-tools-extra/clangd/refactor/tweaks/ExtractVariable.cpp
@@ -368,32 +368,80 @@
return Effect::applyEdit(Result);
}
-// Find the CallExpr whose callee is an ancestor of the DeclRef
+// Find the CallExpr whose callee is the (possibly wrapped) DeclRef
const SelectionTree::Node *getCallExpr(const SelectionTree::Node *DeclRef) {
- // we maintain a stack of all exprs encountered while traversing the
- // selectiontree because the callee of the callexpr can be an ancestor of the
- // DeclRef. e.g. Callee can be an ImplicitCastExpr.
- std::vector<const clang::Expr *> ExprStack;
- for (auto *CurNode = DeclRef; CurNode; CurNode = CurNode->Parent) {
- const Expr *CurExpr = CurNode->ASTNode.get<Expr>();
- if (const CallExpr *CallPar = CurNode->ASTNode.get<CallExpr>()) {
- // check whether the callee of the callexpr is present in Expr stack.
- if (std::find(ExprStack.begin(), ExprStack.end(), CallPar->getCallee()) !=
- ExprStack.end())
- return CurNode;
- return nullptr;
- }
- ExprStack.push_back(CurExpr);
- }
- return nullptr;
+ const SelectionTree::Node &MaybeCallee = DeclRef->outerImplicit();
+ const SelectionTree::Node *MaybeCall = MaybeCallee.Parent;
+ if (!MaybeCall)
+ return nullptr;
+ const CallExpr *CE =
+ llvm::dyn_cast_or_null<CallExpr>(MaybeCall->ASTNode.get<Expr>());
+ if (!CE)
+ return nullptr;
+ if (CE->getCallee() != MaybeCallee.ASTNode.get<Expr>())
+ return nullptr;
+ return MaybeCall;
}
-// check if Expr can be assigned to a variable i.e. is non-void type
-bool canBeAssigned(const SelectionTree::Node *ExprNode) {
- const clang::Expr *Expr = ExprNode->ASTNode.get<clang::Expr>();
- if (const Type *ExprType = Expr->getType().getTypePtrOrNull())
- // FIXME: check if we need to cover any other types
- return !ExprType->isVoidType();
+// 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) {
+ if (!Outer || !Inner)
+ return false;
+ // Blacklist 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.
+ if (const auto* WS = llvm::dyn_cast<WhileStmt>(Outer))
+ return Inner == WS->getBody();
+ if (const auto* DS = llvm::dyn_cast<DoStmt>(Outer))
+ return Inner == DS->getBody();
+ if (const auto* FS = llvm::dyn_cast<ForStmt>(Outer))
+ return Inner == FS->getBody();
+ if (const auto* FS = llvm::dyn_cast<CXXForRangeStmt>(Outer))
+ return Inner == FS->getBody();
+ if (const auto* IS = llvm::dyn_cast<IfStmt>(Outer))
+ return Inner == IS->getThen() || Inner == IS->getElse();
+ // Assume all other cases may be actual expressions.
+ // This includes the important case of subexpressions (where Outer is Expr).
+ return false;
+}
+
+// check if N can and should be extracted (e.g. is not void-typed).
+bool eligibleForExtraction(const SelectionTree::Node *N) {
+ const Expr *E = N->ASTNode.get<Expr>();
+ if (!E)
+ return false;
+
+ // Void expressions can't be assigned to variables.
+ if (const Type *ExprType = E->getType().getTypePtrOrNull())
+ if (ExprType->isVoidType())
+ return false;
+
+ // A plain reference to a name (e.g. variable) isn't worth extracting.
+ // FIXME: really? What if it's e.g. `std::is_same<void, void>::value`?
+ if (llvm::isa<DeclRefExpr>(E) || llvm::isa<MemberExpr>(E))
+ return false;
+
+ // Extracting Exprs like a = 1 gives dummy = a = 1 which isn't useful.
+ // FIXME: we could still hoist the assignment, and leave the variable there?
+ ParsedBinaryOperator BinOp;
+ if (BinOp.parse(*N) && BinaryOperator::isAssignmentOp(BinOp.Kind))
+ return false;
+
+ // We don't want to extract expressions used as statements, that would leave
+ // a `dummy;` around that has no effect.
+ // Unfortunately because the AST doesn't have ExprStmt, we have to check in
+ // this roundabout way.
+ const SelectionTree::Node &OuterImplicit = N->outerImplicit();
+ if (!OuterImplicit.Parent ||
+ childExprIsStmt(OuterImplicit.Parent->ASTNode.get<Stmt>(),
+ OuterImplicit.ASTNode.get<Expr>()))
+ return false;
+
+ // FIXME: ban extracting the RHS of an assignment: `a = [[foo()]]`
return true;
}
@@ -406,32 +454,13 @@
const ASTContext &Ctx) {
if (!N)
return false;
- const clang::Expr *SelectedExpr = N->ASTNode.get<clang::Expr>();
const SelectionTree::Node *TargetNode = N;
- if (!SelectedExpr)
- return false;
- // Extracting Exprs like a = 1 gives dummy = a = 1 which isn't useful.
- if (const BinaryOperator *BinOpExpr =
- dyn_cast_or_null<BinaryOperator>(SelectedExpr)) {
- if (BinOpExpr->isAssignmentOp())
- return false;
- }
- // For function and member function DeclRefs, we look for a parent that is a
- // CallExpr
- if (const DeclRefExpr *DeclRef =
- dyn_cast_or_null<DeclRefExpr>(SelectedExpr)) {
- // Extracting just a variable isn't that useful.
- if (!isa<FunctionDecl>(DeclRef->getDecl()))
- return false;
- TargetNode = getCallExpr(N);
- }
- if (const MemberExpr *Member = dyn_cast_or_null<MemberExpr>(SelectedExpr)) {
- // Extracting just a field member isn't that useful.
- if (!isa<CXXMethodDecl>(Member->getMemberDecl()))
- return false;
- TargetNode = getCallExpr(N);
- }
- if (!TargetNode || !canBeAssigned(TargetNode))
+ // For function and member function DeclRefs, extract the whole call.
+ if (const Expr *E = N->ASTNode.get<clang::Expr>())
+ if (llvm::isa<DeclRefExpr>(E) || llvm::isa<MemberExpr>(E))
+ if (const SelectionTree::Node *Call = getCallExpr(N))
+ TargetNode = Call;
+ if (!eligibleForExtraction(TargetNode))
return false;
Target = llvm::make_unique<ExtractionContext>(TargetNode, SM, Ctx);
return Target->isExtractable();
Index: clang-tools-extra/clangd/Selection.h
===================================================================
--- clang-tools-extra/clangd/Selection.h
+++ clang-tools-extra/clangd/Selection.h
@@ -106,6 +106,9 @@
// If this node is a wrapper with no syntax (e.g. implicit cast), return
// its contents. (If multiple wrappers are present, unwraps all of them).
const Node& ignoreImplicit() const;
+ // If this node is inside a wrapper with no syntax (e.g. implicit cast),
+ // return that wrapper. (If multiple are present, unwraps all of them).
+ const Node& outerImplicit() const;
};
// The most specific common ancestor of all the selected nodes.
// Returns nullptr if the common ancestor is the root.
Index: clang-tools-extra/clangd/Selection.cpp
===================================================================
--- clang-tools-extra/clangd/Selection.cpp
+++ clang-tools-extra/clangd/Selection.cpp
@@ -466,5 +466,11 @@
return *this;
}
+const SelectionTree::Node &SelectionTree::Node::outerImplicit() const {
+ if (Parent && Parent->ASTNode.getSourceRange() == ASTNode.getSourceRange())
+ return Parent->outerImplicit();
+ return *this;
+}
+
} // namespace clangd
} // namespace clang
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
