SureYeaah updated this revision to Diff 209922.
SureYeaah added a comment.
Added fix for selecting the callExpr of a MemberExpr/Function DeclRefExpr
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D64717/new/
https://reviews.llvm.org/D64717
Files:
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
@@ -291,6 +291,7 @@
const char *Input = "struct ^X { int x; int y; }";
EXPECT_THAT(getMessage(ID, Input), ::testing::HasSubstr("0 | int x"));
}
+
TEST(TweakTest, ExtractVariable) {
llvm::StringLiteral ID = "ExtractVariable";
checkAvailable(ID, R"cpp(
@@ -302,7 +303,7 @@
int a = 5 + [[4 ^* ^xyz^()]];
// multivariable initialization
if(1)
- int x = ^1, y = ^a + 1, a = ^1, z = a + 1;
+ int x = ^1, y = [[a + 1]], a = ^1, z = a + 1;
// if without else
if(^1) {}
// if with else
@@ -315,13 +316,13 @@
a = ^4;
else
a = ^5;
- // for loop
+ // for loop
for(a = ^1; a > ^3^+^4; a++)
a = ^2;
- // while
+ // while
while(a < ^1)
- ^a++;
- // do while
+ [[a++]];
+ // do while
do
a = ^1;
while(a < ^3);
@@ -343,22 +344,28 @@
int xyz = ^1;
};
}
+ void v() { return; }
// function default argument
void f(int b = ^1) {
// void expressions
auto i = new int, j = new int;
de^lete i^, del^ete j;
+ [[g]]();
// if
if(1)
int x = 1, y = a + 1, a = 1, z = ^a + 1;
if(int a = 1)
if(^a == 4)
a = ^a ^+ 1;
- // for loop
+ // for loop
for(int a = 1, b = 2, c = 3; ^a > ^b ^+ ^c; ^a++)
a = ^a ^+ 1;
- // lambda
+ // lambda
auto lamb = [&^a, &^b](int r = ^1) {return 1;}
+ // assigment
+ [[a ^= 5]];
+ // Variable DeclRefExpr
+ a = [[b]];
}
)cpp");
// vector of pairs of input and output strings
@@ -412,6 +419,24 @@
R"cpp(void f(int a) {
auto dummy = 1; label: [ [gsl::suppress("type")] ] for (;;) a = dummy;
})cpp"},
+ // MemberExpr
+ {R"cpp(class T {
+ T f() {
+ return [[T().f().f]]();
+ }
+ };)cpp",
+ R"cpp(class T {
+ T f() {
+ auto dummy = T().f().f(); return dummy;
+ }
+ };)cpp"},
+ // Function DeclRefExpr
+ {R"cpp(int f() {
+ return [[f]]();
+ })cpp",
+ R"cpp(int f() {
+ auto dummy = f(); return dummy;
+ })cpp"},
// FIXME: Doesn't work because bug in selection tree
/*{R"cpp(#define PLUS(x) x++
void f(int a) {
@@ -421,8 +446,8 @@
void f(int a) {
auto dummy = a; PLUS(dummy);
})cpp"},*/
- // FIXME: Doesn't work correctly for \[\[clang::uninitialized\]\] int b
- // = 1; since the attr is inside the DeclStmt and the bounds of
+ // FIXME: Wrong result for \[\[clang::uninitialized\]\] int b = 1;
+ // since the attr is inside the DeclStmt and the bounds of
// DeclStmt don't cover the attribute
};
for (const auto &IO : InputOutputs) {
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
@@ -13,6 +13,7 @@
#include "refactor/Tweak.h"
#include "clang/AST/ASTContext.h"
#include "clang/AST/Expr.h"
+#include "clang/AST/ExprCXX.h"
#include "clang/AST/OperationKinds.h"
#include "clang/AST/RecursiveASTVisitor.h"
#include "clang/AST/Stmt.h"
@@ -37,18 +38,16 @@
const ASTContext &Ctx);
const clang::Expr *getExpr() const { return Expr; }
const SelectionTree::Node *getExprNode() const { return ExprNode; }
- bool isExtractable() const { return Extractable; }
// Generate Replacement for replacing selected expression with given VarName
tooling::Replacement replaceWithVar(llvm::StringRef VarName) const;
// Generate Replacement for declaring the selected Expr as a new variable
tooling::Replacement insertDeclaration(llvm::StringRef VarName) const;
+ const clang::Stmt *InsertionPoint = nullptr;
private:
- bool Extractable = false;
const clang::Expr *Expr;
const SelectionTree::Node *ExprNode;
// Stmt before which we will extract
- const clang::Stmt *InsertionPoint = nullptr;
const SourceManager &SM;
const ASTContext &Ctx;
// Decls referenced in the Expr
@@ -77,29 +76,13 @@
return Visitor.ReferencedDecls;
}
-// An expr is not extractable if it's null or an expression of type void
-// FIXME: Ignore assignment (a = 1) Expr since it is extracted as dummy = a =
-static bool isExtractableExpr(const clang::Expr *Expr) {
- if (Expr) {
- const Type *ExprType = Expr->getType().getTypePtrOrNull();
- // FIXME: check if we need to cover any other types
- if (ExprType)
- return !ExprType->isVoidType();
- }
- return false;
-}
-
ExtractionContext::ExtractionContext(const SelectionTree::Node *Node,
const SourceManager &SM,
const ASTContext &Ctx)
: ExprNode(Node), SM(SM), Ctx(Ctx) {
Expr = Node->ASTNode.get<clang::Expr>();
- if (isExtractableExpr(Expr)) {
- ReferencedDecls = computeReferencedDecls(Expr);
- InsertionPoint = computeInsertionPoint();
- if (InsertionPoint)
- Extractable = true;
- }
+ ReferencedDecls = computeReferencedDecls(Expr);
+ InsertionPoint = computeInsertionPoint();
}
// checks whether extracting before InsertionPoint will take a
@@ -121,9 +104,9 @@
// the current Stmt. We ALWAYS insert before a Stmt whose parent is a
// CompoundStmt
//
-
// FIXME: Extraction from switch and case statements
// FIXME: Doens't work for FoldExpr
+// FIXME: Ensure extraction from loops doesn't change semantics
const clang::Stmt *ExtractionContext::computeInsertionPoint() const {
// returns true if we can extract before InsertionPoint
auto CanExtractOutside =
@@ -209,6 +192,9 @@
return "Extract subexpression to variable";
}
Intent intent() const override { return Refactor; }
+ // Compute the extraction context for the Selection
+ void computeExtractionContext(const SelectionTree::Node *N,
+ const SourceManager &SM, const ASTContext &Ctx);
private:
// the expression to extract
@@ -219,10 +205,8 @@
const ASTContext &Ctx = Inputs.AST.getASTContext();
const SourceManager &SM = Inputs.AST.getSourceManager();
const SelectionTree::Node *N = Inputs.ASTSelection.commonAncestor();
- if (!N)
- return false;
- Target = llvm::make_unique<ExtractionContext>(N, SM, Ctx);
- return Target->isExtractable();
+ computeExtractionContext(N, SM, Ctx);
+ return Target && Target->InsertionPoint;
}
Expected<Tweak::Effect> ExtractVariable::apply(const Selection &Inputs) {
@@ -238,6 +222,56 @@
return Effect::applyEdit(Result);
}
+// Returns a parent node of Expr type T
+template <typename T>
+const SelectionTree::Node *
+getParentExprOfType(const SelectionTree::Node *Node) {
+ for (auto *ParNode = Node->Parent; ParNode && ParNode->ASTNode.get<Expr>();
+ ParNode = ParNode->Parent) {
+ if (ParNode->ASTNode.get<T>())
+ return ParNode;
+ }
+ return nullptr;
+}
+
+// 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();
+ return true;
+}
+
+void ExtractVariable::computeExtractionContext(const SelectionTree::Node *N,
+ const SourceManager &SM,
+ const ASTContext &Ctx) {
+ const clang::Expr *SelectedExpr = N->ASTNode.get<clang::Expr>();
+ const SelectionTree::Node *TargetNode = N;
+ if (!SelectedExpr)
+ return;
+ // 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->getOpcode() == BinaryOperatorKind::BO_Assign)
+ return;
+ }
+ // For function declRefs, we look for a parent that is a CallExpr
+ if (const DeclRefExpr *DeclRef =
+ dyn_cast_or_null<DeclRefExpr>(SelectedExpr)) {
+ // Extracting a variable reference is useless.
+ if (!isa<FunctionDecl>(DeclRef->getDecl()))
+ return;
+ TargetNode = getParentExprOfType<CallExpr>(N);
+ }
+ // look for the call expr parent if the selected expr is a MemberExpr
+ if (dyn_cast_or_null<clang::MemberExpr>(SelectedExpr)) {
+ TargetNode = getParentExprOfType<CXXMemberCallExpr>(N);
+ }
+ if (TargetNode && canBeAssigned(TargetNode))
+ Target = llvm::make_unique<ExtractionContext>(TargetNode, SM, Ctx);
+}
+
} // namespace
} // namespace clangd
} // namespace clang
_______________________________________________
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
