Index: clang-tools-extra/trunk/clangd/Selection.h =================================================================== --- clang-tools-extra/trunk/clangd/Selection.h +++ clang-tools-extra/trunk/clangd/Selection.h @@ -110,6 +110,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/trunk/clangd/Selection.cpp =================================================================== --- clang-tools-extra/trunk/clangd/Selection.cpp +++ clang-tools-extra/trunk/clangd/Selection.cpp @@ -510,5 +510,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 Index: clang-tools-extra/trunk/clangd/refactor/tweaks/ExtractVariable.cpp =================================================================== --- clang-tools-extra/trunk/clangd/refactor/tweaks/ExtractVariable.cpp +++ clang-tools-extra/trunk/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 ExprStack; - for (auto *CurNode = DeclRef; CurNode; CurNode = CurNode->Parent) { - const Expr *CurExpr = CurNode->ASTNode.get(); - if (const CallExpr *CallPar = CurNode->ASTNode.get()) { - // 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(MaybeCall->ASTNode.get()); + if (!CE) + return nullptr; + if (CE->getCallee() != MaybeCallee.ASTNode.get()) + return nullptr; + return MaybeCall; +} + +// 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(Outer)) + return true; + if (llvm::isa(Outer)) + return true; + // Control flow statements use condition etc, but not the body. + if (const auto* WS = llvm::dyn_cast(Outer)) + return Inner == WS->getBody(); + if (const auto* DS = llvm::dyn_cast(Outer)) + return Inner == DS->getBody(); + if (const auto* FS = llvm::dyn_cast(Outer)) + return Inner == FS->getBody(); + if (const auto* FS = llvm::dyn_cast(Outer)) + return Inner == FS->getBody(); + if (const auto* IS = llvm::dyn_cast(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(); + if (!E) + return false; -// 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(); - if (const Type *ExprType = Expr->getType().getTypePtrOrNull()) - // FIXME: check if we need to cover any other types - return !ExprType->isVoidType(); + // 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::value`? + if (llvm::isa(E) || llvm::isa(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(), + OuterImplicit.ASTNode.get())) + 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(); 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(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(SelectedExpr)) { - // Extracting just a variable isn't that useful. - if (!isa(DeclRef->getDecl())) - return false; - TargetNode = getCallExpr(N); - } - if (const MemberExpr *Member = dyn_cast_or_null(SelectedExpr)) { - // Extracting just a field member isn't that useful. - if (!isa(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()) + if (llvm::isa(E) || llvm::isa(E)) + if (const SelectionTree::Node *Call = getCallExpr(N)) + TargetNode = Call; + if (!eligibleForExtraction(TargetNode)) return false; Target = llvm::make_unique(TargetNode, SM, Ctx); return Target->isExtractable(); Index: clang-tools-extra/trunk/clangd/unittests/SelectionTests.cpp =================================================================== --- clang-tools-extra/trunk/clangd/unittests/SelectionTests.cpp +++ clang-tools-extra/trunk/clangd/unittests/SelectionTests.cpp @@ -361,6 +361,8 @@ EXPECT_EQ("CXXConstructExpr", nodeKind(Str->Parent->Parent)); EXPECT_EQ(Str, &Str->Parent->Parent->ignoreImplicit()) << "Didn't unwrap " << nodeKind(&Str->Parent->Parent->ignoreImplicit()); + + EXPECT_EQ("CXXConstructExpr", nodeKind(&Str->outerImplicit())); } } // namespace Index: clang-tools-extra/trunk/clangd/unittests/TweakTests.cpp =================================================================== --- clang-tools-extra/trunk/clangd/unittests/TweakTests.cpp +++ clang-tools-extra/trunk/clangd/unittests/TweakTests.cpp @@ -249,7 +249,7 @@ a = [[2]]; // while while(a < [[1]]) - [[a++]]; + a = [[1]]; // do while do a = [[1]]; @@ -293,11 +293,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: @@ -340,7 +343,7 @@ // Macros {R"cpp(#define PLUS(x) x++ void f(int a) { - PLUS([[1+a]]); + int y = PLUS([[1+a]]); })cpp", /*FIXME: It should be extracted like this. R"cpp(#define PLUS(x) x++ @@ -349,7 +352,7 @@ })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;}