SureYeaah updated this revision to Diff 208174.
SureYeaah marked 21 inline comments as done.
SureYeaah added a comment.
Added whitelist for computeInsertionPoint
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D63773/new/
https://reviews.llvm.org/D63773
Files:
clang-tools-extra/clangd/refactor/tweaks/CMakeLists.txt
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
@@ -239,13 +239,13 @@
checkNotAvailable(ID, "/*c^omment*/ int foo() return 2 ^ + 2; }");
const char *Input = "int x = 2 ^+ 2;";
- auto result = getMessage(ID, Input);
- EXPECT_THAT(result, ::testing::HasSubstr("BinaryOperator"));
- EXPECT_THAT(result, ::testing::HasSubstr("'+'"));
- EXPECT_THAT(result, ::testing::HasSubstr("|-IntegerLiteral"));
- EXPECT_THAT(result,
+ auto Result = getMessage(ID, Input);
+ EXPECT_THAT(Result, ::testing::HasSubstr("BinaryOperator"));
+ EXPECT_THAT(Result, ::testing::HasSubstr("'+'"));
+ EXPECT_THAT(Result, ::testing::HasSubstr("|-IntegerLiteral"));
+ EXPECT_THAT(Result,
::testing::HasSubstr("<col:9> 'int' 2\n`-IntegerLiteral"));
- EXPECT_THAT(result, ::testing::HasSubstr("<col:13> 'int' 2"));
+ EXPECT_THAT(Result, ::testing::HasSubstr("<col:13> 'int' 2"));
}
TEST(TweakTest, ShowSelectionTree) {
@@ -277,6 +277,136 @@
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(
+ int xyz() {
+ // return statement
+ return ^1;
+ }
+ void f() {
+ int a = 5 + [[4 ^* ^xyz^()]];
+ // multivariable initialization
+ if(1)
+ int x = ^1, y = ^a + 1, a = ^1, z = a + 1;
+ // if without else
+ if(^1) {}
+ // if with else
+ if(a < ^3)
+ if(a == ^4)
+ a = ^5;
+ else
+ a = ^6;
+ else if (a < ^4)
+ a = ^4;
+ else
+ a = ^5;
+ // for loop
+ for(a = ^1; a > ^3^+^4; a++)
+ a = ^2;
+ // while
+ while(a < ^1)
+ ^a++;
+ // do while
+ do
+ a = ^1;
+ while(a < ^3);
+ }
+ )cpp");
+ checkNotAvailable(ID, R"cpp(
+ int xyz(int a = ^1) {
+ return 1;
+ class T {
+ T(int a = ^1) {};
+ int xyz = ^1;
+ };
+ }
+ // function default argument
+ void f(int b = ^1) {
+ // invalid expression checking;
+ auto i = new int, j = new int;
+ de^lete i^, del^ete j;
+ // 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(int a = 1, b = 2, c = 3; ^a > ^b ^+ ^c; ^a++)
+ a = ^a ^+ 1;
+ // lambda
+ auto lamb = [&^a, &^b](int r = ^1) {return 1;}
+ }
+ )cpp");
+ // vector of pairs of input and output strings
+ const std::vector<std::pair<llvm::StringLiteral, llvm::StringLiteral>>
+ InputOutputs = {
+ // extraction from variable declaration/assignment
+ {R"cpp(void varDecl() {
+ int a = 5 * (4 + (3 [[- 1)]]);
+ })cpp",
+ R"cpp(void varDecl() {
+ auto dummy = (3 - 1); int a = 5 * (4 + dummy);
+ })cpp"},
+ // FIXME: extraction from switch case
+ /*{R"cpp(void f(int a) {
+ if(1)
+ while(a < 1)
+ switch (1) {
+ case 1:
+ a = [[1 + 2]];
+ break;
+ default:
+ break;
+ }
+ })cpp",
+ R"cpp(void f(int a) {
+ auto dummy = 1 + 2; if(1)
+ while(a < 1)
+ switch (1) {
+ case 1:
+ a = dummy;
+ break;
+ default:
+ break;
+ }
+ })cpp"},*/
+ // ensure InsertionPoint isn't inside a macro
+ {R"cpp(#define LOOP(x) {int a = x + 1;}
+ void f(int a) {
+ if(1)
+ LOOP(5 + ^3)
+ })cpp",
+ R"cpp(#define LOOP(x) {int a = x + 1;}
+ void f(int a) {
+ auto dummy = 3; if(1)
+ LOOP(5 + dummy)
+ })cpp"},
+ // label and attribute testing
+ {R"cpp(void f(int a) {
+ label: [ [gsl::suppress("type")] ] for (;;) a = ^1;
+ })cpp",
+ R"cpp(void f(int a) {
+ auto dummy = 1; label: [ [gsl::suppress("type")] ] for (;;) a = dummy;
+ })cpp"},
+ // FIXME: Doesn't work because bug in selection tree
+ /*{R"cpp(#define PLUS(x) x++
+ void f(int a) {
+ PLUS(^a);
+ })cpp",
+ R"cpp(#define PLUS(x) x++
+ 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
+ // DeclStmt don't cover the attribute
+ };
+ for (const auto &IO : InputOutputs) {
+ checkTransform(ID, IO.first, IO.second);
+ }
+}
} // namespace
} // namespace clangd
Index: clang-tools-extra/clangd/refactor/tweaks/ExtractVariable.cpp
===================================================================
--- /dev/null
+++ clang-tools-extra/clangd/refactor/tweaks/ExtractVariable.cpp
@@ -0,0 +1,238 @@
+//===--- ExtractVariable.cpp ------------------------------------*- C++-*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+#include "ClangdUnit.h"
+#include "Logger.h"
+#include "Protocol.h"
+#include "Selection.h"
+#include "SourceCode.h"
+#include "refactor/Tweak.h"
+#include "clang/AST/ASTContext.h"
+#include "clang/AST/Expr.h"
+#include "clang/AST/OperationKinds.h"
+#include "clang/AST/RecursiveASTVisitor.h"
+#include "clang/AST/Stmt.h"
+#include "clang/Basic/LangOptions.h"
+#include "clang/Basic/SourceLocation.h"
+#include "clang/Basic/SourceManager.h"
+#include "clang/Tooling/Core/Replacement.h"
+#include "llvm/ADT/None.h"
+#include "llvm/ADT/SmallVector.h"
+#include "llvm/ADT/StringRef.h"
+#include "llvm/Support/Casting.h"
+#include "llvm/Support/Error.h"
+
+namespace clang {
+namespace clangd {
+namespace {
+// information regarding the Expr that is being extracted
+class ExtractionContext {
+public:
+ ExtractionContext(const SelectionTree::Node *Node, const SourceManager &SM,
+ const ASTContext &Ctx);
+ // return the expr
+ const clang::Expr *getExpr() const { return Expr; }
+ // return the SelectionTree node for the Expr
+ const SelectionTree::Node *getExprNode() const { return ExprNode; }
+ // returns true if the Expr can be extracted
+ 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;
+
+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;
+ // vector of all Decls referenced in the Expr
+ std::vector<clang::Decl *> ReferencedDecls;
+ // returns true if the Expr doesn't reference any variable declared in scope
+ bool exprIsValidOutside(const clang::Stmt *Scope) const;
+ // computes the Stmt before which we will extract out Expr
+ const clang::Stmt *computeInsertionPoint() const;
+};
+
+// Returns all the Decls referenced inside the given Expr
+static std::vector<clang::Decl *>
+computeReferencedDecls(const clang::Expr *Expr) {
+ // RAV subclass to find all DeclRefs in a given Stmt
+ class FindDeclRefsVisitor
+ : public clang::RecursiveASTVisitor<FindDeclRefsVisitor> {
+ public:
+ std::vector<Decl *> ReferencedDecls;
+ bool VisitDeclRefExpr(DeclRefExpr *DeclRef) { // NOLINT
+ ReferencedDecls.push_back(DeclRef->getDecl());
+ return true;
+ }
+ };
+ FindDeclRefsVisitor Visitor;
+ Visitor.TraverseStmt(const_cast<Stmt *>(dyn_cast<Stmt>(Expr)));
+ return Visitor.ReferencedDecls;
+}
+
+// An expr is not extractable if it's null, a delete expression or a comma
+// separated expression
+static bool isExtractableExpr(const clang::Expr *Expr) {
+ if (Expr) {
+ if (const BinaryOperator *BinOpExpr =
+ dyn_cast_or_null<BinaryOperator>(Expr))
+ return BinOpExpr->getOpcode() != BinaryOperatorKind::BO_Comma;
+ return !isa<CXXDeleteExpr>(Expr);
+ }
+ 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);
+ if ((InsertionPoint = computeInsertionPoint()))
+ Extractable = true;
+ }
+}
+
+// checks whether extracting before InsertionPoint will take a
+// variable out of scope
+bool ExtractionContext::exprIsValidOutside(const clang::Stmt *Scope) const {
+ SourceLocation ScopeBegin = Scope->getBeginLoc();
+ SourceLocation ScopeEnd = Scope->getEndLoc();
+ for (const Decl *ReferencedDecl : ReferencedDecls) {
+ if (SM.isPointWithin(ReferencedDecl->getBeginLoc(), ScopeBegin, ScopeEnd) &&
+ SM.isPointWithin(ReferencedDecl->getEndLoc(), ScopeBegin, ScopeEnd))
+ return false;
+ }
+ return true;
+}
+
+// Return the Stmt before which we need to insert the extraction.
+// To find the Stmt, we go up the AST Tree and if the Parent of the current
+// Stmt is a CompoundStmt, we can extract inside this CompoundStmt just before
+// the current Stmt. We ALWAYS insert before a Stmt whose parent is a
+// CompoundStmt
+//
+
+// FIXME: Ignore assignment (a = 1) Expr since it is extracted as dummy = a =
+// FIXME: Extraction from switch and case statements
+// FIXME: Doens't work for FoldExpr
+const clang::Stmt *ExtractionContext::computeInsertionPoint() const {
+ // returns true if we can extract before InsertionPoint
+ auto CanExtractOutside =
+ [](const SelectionTree::Node *InsertionPoint) -> bool {
+ if (const clang::Stmt *Stmt = InsertionPoint->ASTNode.get<clang::Stmt>()) {
+ // Allow all expressions except LambdaExpr
+ if (isa<clang::Expr>(Stmt))
+ return !isa<LambdaExpr>(Stmt);
+ return isa<AttributedStmt>(Stmt) || isa<CompoundStmt>(Stmt) ||
+ isa<DeclStmt>(Stmt) || isa<DoStmt>(Stmt) || isa<ForStmt>(Stmt) ||
+ isa<IfStmt>(Stmt) || isa<LabelStmt>(Stmt) ||
+ isa<ReturnStmt>(Stmt) || isa<WhileStmt>(Stmt);
+ }
+ if (InsertionPoint->ASTNode.get<VarDecl>())
+ return true;
+ return false;
+ };
+ for (const SelectionTree::Node *CurNode = getExprNode();
+ CurNode->Parent && CanExtractOutside(CurNode);
+ CurNode = CurNode->Parent) {
+ const clang::Stmt *CurInsertionPoint = CurNode->ASTNode.get<Stmt>();
+ // give up if extraction will take a variable out of scope
+ if (CurInsertionPoint && !exprIsValidOutside(CurInsertionPoint))
+ break;
+ if (const clang::Stmt *CurParent = CurNode->Parent->ASTNode.get<Stmt>()) {
+ if (isa<CompoundStmt>(CurParent)) {
+ // Ensure we don't write inside a macro.
+ if (CurParent->getBeginLoc().isMacroID())
+ continue;
+ return CurInsertionPoint;
+ }
+ }
+ }
+ return nullptr;
+}
+// returns the replacement for substituting the extraction with VarName
+tooling::Replacement
+ExtractionContext::replaceWithVar(llvm::StringRef VarName) const {
+ const llvm::Optional<SourceRange> ExtractionRng =
+ toHalfOpenFileRange(SM, Ctx.getLangOpts(), getExpr()->getSourceRange());
+ unsigned ExtractionLength = SM.getFileOffset(ExtractionRng->getEnd()) -
+ SM.getFileOffset(ExtractionRng->getBegin());
+ return tooling::Replacement(SM, ExtractionRng->getBegin(), ExtractionLength,
+ VarName);
+}
+// returns the Replacement for declaring a new variable storing the extraction
+tooling::Replacement
+ExtractionContext::insertDeclaration(llvm::StringRef VarName) const {
+ const llvm::Optional<SourceRange> ExtractionRng =
+ toHalfOpenFileRange(SM, Ctx.getLangOpts(), getExpr()->getSourceRange());
+ assert(ExractionRng && "ExtractionRng should not be null");
+ llvm::StringRef ExtractionCode = toSourceCode(SM, *ExtractionRng);
+ const SourceLocation InsertionLoc =
+ toHalfOpenFileRange(SM, Ctx.getLangOpts(),
+ InsertionPoint->getSourceRange())
+ ->getBegin();
+ // FIXME: Replace auto with explicit type and add &/&& as necessary
+ std::string ExtractedVarDecl = std::string("auto ") + VarName.str() + " = " +
+ ExtractionCode.str() + "; ";
+ return tooling::Replacement(SM, InsertionLoc, 0, ExtractedVarDecl);
+}
+
+/// Extracts an expression to the variable dummy
+/// Before:
+/// int x = 5 + 4 * 3;
+/// ^^^^^
+/// After:
+/// auto dummy = 5 + 4;
+/// int x = dummy * 3;
+class ExtractVariable : public Tweak {
+public:
+ const char *id() const override final;
+ bool prepare(const Selection &Inputs) override;
+ Expected<Effect> apply(const Selection &Inputs) override;
+ std::string title() const override {
+ return "Extract subexpression to variable";
+ }
+ Intent intent() const override { return Refactor; }
+
+private:
+ // the expression to extract
+ std::unique_ptr<ExtractionContext> Target;
+};
+REGISTER_TWEAK(ExtractVariable)
+bool ExtractVariable::prepare(const Selection &Inputs) {
+ 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();
+}
+
+Expected<Tweak::Effect> ExtractVariable::apply(const Selection &Inputs) {
+ tooling::Replacements Result;
+ // FIXME: get variable name from user or suggest based on type
+ std::string VarName = "dummy";
+ // insert new variable declaration
+ if (auto Err = Result.add(Target->insertDeclaration(VarName)))
+ return std::move(Err);
+ // replace expression with variable name
+ if (auto Err = Result.add(Target->replaceWithVar(VarName)))
+ return std::move(Err);
+ return Effect::applyEdit(Result);
+}
+
+} // namespace
+} // namespace clangd
+} // namespace clang
Index: clang-tools-extra/clangd/refactor/tweaks/CMakeLists.txt
===================================================================
--- clang-tools-extra/clangd/refactor/tweaks/CMakeLists.txt
+++ clang-tools-extra/clangd/refactor/tweaks/CMakeLists.txt
@@ -15,6 +15,7 @@
DumpAST.cpp
RawStringLiteral.cpp
SwapIfBranches.cpp
+ ExtractVariable.cpp
LINK_LIBS
clangAST
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
