sammccall updated this revision to Diff 211679.
sammccall added a comment.
Rather than handling arbitrary trees, exploit the fact that the operator is
left-associative.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D65139/new/
https://reviews.llvm.org/D65139
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/SelectionTests.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
@@ -444,6 +444,47 @@
// 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
+
+ // Binary subexpressions
+ {R"cpp(void f() {
+ int x = 1 + [[2 + 3 + 4]] + 5;
+ })cpp",
+ R"cpp(void f() {
+ auto dummy = 2 + 3 + 4; int x = 1 + dummy + 5;
+ })cpp"},
+ // Non-associative operations have no special support
+ {R"cpp(void f() {
+ int x = 1 - [[2 - 3 - 4]] - 5;
+ })cpp",
+ R"cpp(void f() {
+ auto dummy = 1 - 2 - 3 - 4; int x = dummy - 5;
+ })cpp"},
+ // A mix of associative operators isn't associative.
+ {R"cpp(void f() {
+ int x = 1 * [[2 + 3 * 4]] + 5;
+ })cpp",
+ R"cpp(void f() {
+ auto dummy = 1 * 2 + 3 * 4; int x = dummy + 5;
+ })cpp"},
+ // Overloaded operators are supported, we assume associativity
+ // as if they were built-in.
+ {R"cpp(struct S {
+ S(int);
+ };
+ S operator+(S, S);
+
+ void f() {
+ S x = S(1) + [[S(2) + S(3) + S(4)]] + S(5);
+ })cpp",
+ R"cpp(struct S {
+ S(int);
+ };
+ S operator+(S, S);
+
+ void f() {
+ auto dummy = S(2) + S(3) + S(4); S x = S(1) + dummy + S(5);
+ })cpp"},
+
};
for (const auto &IO : InputOutputs) {
checkTransform(ID, IO.first, IO.second);
Index: clang-tools-extra/clangd/unittests/SelectionTests.cpp
===================================================================
--- clang-tools-extra/clangd/unittests/SelectionTests.cpp
+++ clang-tools-extra/clangd/unittests/SelectionTests.cpp
@@ -335,6 +335,23 @@
}
}
+TEST(SelectionTest, Implicit) {
+ const char* Test = R"cpp(
+ struct S { S(const char*); };
+ int f(S);
+ int x = f("^");
+ )cpp";
+ auto AST = TestTU::withCode(Annotations(Test).code()).build();
+ auto T = makeSelectionTree(Test, AST);
+
+ const SelectionTree::Node *Str = T.commonAncestor();
+ EXPECT_EQ("StringLiteral", nodeKind(Str)) << "Implicit selected?";
+ EXPECT_EQ("ImplicitCastExpr", nodeKind(Str->Parent));
+ EXPECT_EQ("CXXConstructExpr", nodeKind(Str->Parent->Parent));
+ EXPECT_EQ(Str, &Str->Parent->Parent->ignoreImplicit())
+ << "Didn't unwrap " << nodeKind(&Str->Parent->Parent->ignoreImplicit());
+}
+
} // namespace
} // namespace clangd
} // namespace clang
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,7 +13,9 @@
#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/PrettyPrinter.h"
#include "clang/AST/RecursiveASTVisitor.h"
#include "clang/AST/Stmt.h"
#include "clang/AST/StmtCXX.h"
@@ -26,6 +28,7 @@
#include "llvm/ADT/StringRef.h"
#include "llvm/Support/Casting.h"
#include "llvm/Support/Error.h"
+#include "llvm/Support/raw_ostream.h"
namespace clang {
namespace clangd {
@@ -38,10 +41,14 @@
const clang::Expr *getExpr() const { return Expr; }
const SelectionTree::Node *getExprNode() const { return ExprNode; }
bool isExtractable() const { return Extractable; }
+ // The half-open range for the expression to be extracted.
+ SourceRange getExtractionChars() const;
// Generate Replacement for replacing selected expression with given VarName
- tooling::Replacement replaceWithVar(llvm::StringRef VarName) const;
+ tooling::Replacement replaceWithVar(SourceRange Chars,
+ llvm::StringRef VarName) const;
// Generate Replacement for declaring the selected Expr as a new variable
- tooling::Replacement insertDeclaration(llvm::StringRef VarName) const;
+ tooling::Replacement insertDeclaration(llvm::StringRef VarName,
+ SourceRange InitChars) const;
private:
bool Extractable = false;
@@ -166,23 +173,20 @@
}
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);
+ExtractionContext::replaceWithVar(SourceRange Chars,
+ llvm::StringRef VarName) const {
+ unsigned ExtractionLength =
+ SM.getFileOffset(Chars.getEnd()) - SM.getFileOffset(Chars.getBegin());
+ return tooling::Replacement(SM, Chars.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(ExtractionRng && "ExtractionRng should not be null");
- llvm::StringRef ExtractionCode = toSourceCode(SM, *ExtractionRng);
+ExtractionContext::insertDeclaration(llvm::StringRef VarName,
+ SourceRange InitializerChars) const {
+ llvm::StringRef ExtractionCode = toSourceCode(SM, InitializerChars);
const SourceLocation InsertionLoc =
toHalfOpenFileRange(SM, Ctx.getLangOpts(),
InsertionPoint->getSourceRange())
@@ -193,6 +197,142 @@
return tooling::Replacement(SM, InsertionLoc, 0, ExtractedVarDecl);
}
+// Helpers for handling "binary subexpressions" like a + [[b + c]] + d.
+//
+// These are special, because the formal AST doesn't match what users expect:
+// - the AST is ((a + b) + c) + d, so the ancestor expression is `a + b + c`.
+// - but extracting `b + c` is reasonable, as + is (mathematically) associative.
+//
+// So we try to support these cases with some restrictions:
+// - the operator must be associative
+// - no mixing of operators is allowed
+// - we don't look inside macro expansions in the subexpressions
+// - we only adjust the extracted range, so references in the unselected parts
+// of the AST expression (e.g. `a`) are still considered referenced for
+// the purposes of calculating the insertion point.
+// FIXME: it would be nice to exclude these references, by micromanaging
+// the computeReferencedDecls() calls around the binary operator tree.
+
+// Information extracted about a binary operator encounted in a SelectionTree.
+// It can represent either an overloaded or built-in operator.
+struct ParsedBinaryOperator {
+ BinaryOperatorKind Kind;
+ SourceLocation ExprLoc;
+ llvm::SmallVector<const SelectionTree::Node*, 8> SelectedOperands;
+
+ // If N is a binary operator, populate this and return true.
+ bool parse(const SelectionTree::Node &N) {
+ SelectedOperands.clear();
+
+ if (const BinaryOperator *Op =
+ llvm::dyn_cast_or_null<BinaryOperator>(N.ASTNode.get<Expr>())) {
+ Kind = Op->getOpcode();
+ ExprLoc = Op->getExprLoc();
+ SelectedOperands = N.Children;
+ return true;
+ }
+ if (const CXXOperatorCallExpr *Op =
+ llvm::dyn_cast_or_null<CXXOperatorCallExpr>(
+ N.ASTNode.get<Expr>())) {
+ if (!Op->isInfixBinaryOp())
+ return false;
+
+ Kind = BinaryOperator::getOverloadedOpcode(Op->getOperator());
+ ExprLoc = Op->getExprLoc();
+ for (const auto* Child : N.Children) {
+ const Expr *E = Child->ASTNode.get<Expr>();
+ assert(E && "callee and args should be Exprs!");
+ if (E == Op->getArg(0) || E == Op->getArg(1))
+ SelectedOperands.push_back(Child);
+ }
+ return true;
+ }
+ return false;
+ }
+
+ bool associative() const {
+ // Must also be left-associative, or update getBinaryOperatorRange()!
+ switch (Kind) {
+ case BO_Add:
+ case BO_Mul:
+ case BO_And:
+ case BO_Or:
+ case BO_Xor:
+ case BO_LAnd:
+ case BO_LOr:
+ return true;
+ default:
+ return false;
+ }
+ }
+
+ bool crossesMacroBoundary(const SourceManager &SM) {
+ FileID F = SM.getFileID(ExprLoc);
+ for (const SelectionTree::Node *Child : SelectedOperands)
+ if (SM.getFileID(Child->ASTNode.getSourceRange().getBegin()) != F)
+ return true;
+ return false;
+ }
+};
+
+// If have an associative operator at the top level, then we must find
+// the start point (rightmost in LHS) and end point (leftmost in RHS).
+// We can only descend into subtrees where the operator matches.
+//
+// e.g. for a + [[b + c]] + d
+// +
+// / \
+// N-> + d
+// / \
+// + c <- End
+// / \
+// a b <- Start
+const SourceRange getBinaryOperatorRange(const SelectionTree::Node &N,
+ const SourceManager &SM,
+ const LangOptions &LangOpts) {
+ // If N is not a suitable binary operator, bail out.
+ ParsedBinaryOperator Op;
+ if (!Op.parse(N.ignoreImplicit()) || !Op.associative() ||
+ Op.crossesMacroBoundary(SM) || Op.SelectedOperands.size() != 2)
+ return SourceRange();
+ BinaryOperatorKind OuterOp = Op.Kind;
+
+ // Because the tree we're interested in contains only one operator type, and
+ // all eligible operators are left-associative, the shape of the tree is
+ // very restricted: it's a linked list along the left edges.
+ // This simplifies our implementation.
+ const SelectionTree::Node *Start = Op.SelectedOperands.front(); // LHS
+ const SelectionTree::Node *End = Op.SelectedOperands.back(); // RHS
+ // End is already correct, Start needs to be pushed down to the right spot.
+ while (Op.parse(Start->ignoreImplicit()) && Op.Kind == OuterOp &&
+ !Op.crossesMacroBoundary(SM)) {
+ assert(!Op.SelectedOperands.empty() && "got only operator on one side!");
+ if (Op.SelectedOperands.size() == 1) { // Only Op.RHS selected
+ Start = Op.SelectedOperands.back();
+ break;
+ }
+ // Op.LHS (partially) selected, so descend into it.
+ Start = Op.SelectedOperands.front();
+ }
+
+ return SourceRange(
+ toHalfOpenFileRange(SM, LangOpts, Start->ASTNode.getSourceRange())
+ ->getBegin(),
+ toHalfOpenFileRange(SM, LangOpts, End->ASTNode.getSourceRange())
+ ->getEnd());
+}
+
+SourceRange ExtractionContext::getExtractionChars() const {
+ // Special case: we're extracting an associative binary subexpression.
+ SourceRange BinaryOperatorRange =
+ getBinaryOperatorRange(*ExprNode, SM, Ctx.getLangOpts());
+ if (BinaryOperatorRange.isValid())
+ return BinaryOperatorRange;
+
+ // Usual case: we're extracting the whole expression.
+ return *toHalfOpenFileRange(SM, Ctx.getLangOpts(), Expr->getSourceRange());
+}
+
/// Extracts an expression to the variable dummy
/// Before:
/// int x = 5 + 4 * 3;
@@ -230,11 +370,12 @@
tooling::Replacements Result;
// FIXME: get variable name from user or suggest based on type
std::string VarName = "dummy";
+ SourceRange Range = Target->getExtractionChars();
// insert new variable declaration
- if (auto Err = Result.add(Target->insertDeclaration(VarName)))
+ if (auto Err = Result.add(Target->insertDeclaration(VarName, Range)))
return std::move(Err);
// replace expression with variable name
- if (auto Err = Result.add(Target->replaceWithVar(VarName)))
+ if (auto Err = Result.add(Target->replaceWithVar(Range, VarName)))
return std::move(Err);
return Effect::applyEdit(Result);
}
Index: clang-tools-extra/clangd/Selection.h
===================================================================
--- clang-tools-extra/clangd/Selection.h
+++ clang-tools-extra/clangd/Selection.h
@@ -96,6 +96,9 @@
// Walk up the AST to get the DeclContext of this Node,
// which is not the node itself.
const DeclContext& getDeclContext() const;
+ // 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;
};
// The most specific common ancestor of all the selected nodes.
// If there is no selection, this is nullptr.
Index: clang-tools-extra/clangd/Selection.cpp
===================================================================
--- clang-tools-extra/clangd/Selection.cpp
+++ clang-tools-extra/clangd/Selection.cpp
@@ -430,5 +430,12 @@
llvm_unreachable("A tree must always be rooted at TranslationUnitDecl.");
}
+const SelectionTree::Node &SelectionTree::Node::ignoreImplicit() const {
+ if (Children.size() == 1 &&
+ Children.front()->ASTNode.getSourceRange() == ASTNode.getSourceRange())
+ return Children.front()->ignoreImplicit();
+ 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
