sammccall updated this revision to Diff 211521.
sammccall marked an inline comment as done.
sammccall added a comment.
Address minor comments, but not tree-traversal simplification yet.
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,162 @@
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;
+ bool OperatorSelected = false;
+ llvm::SmallVector<const SelectionTree::Node*, 8> SelectedOperands;
+
+ // If N is a binary operator, populate this and return true.
+ bool parse(const SelectionTree::Node &N) {
+ if (const BinaryOperator *Op =
+ llvm::dyn_cast_or_null<BinaryOperator>(N.ASTNode.get<Expr>())) {
+ Kind = Op->getOpcode();
+ // FIXME: this could also be whitespace around the operand.
+ OperatorSelected = N.Selected;
+ 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());
+ 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);
+ else
+ OperatorSelected = true;
+ }
+ return true;
+ }
+ return false;
+ }
+
+ bool associative() const {
+ 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;
+ }
+ }
+};
+
+// 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 and the
+// operator itself is not selected.
+//
+// e.g. for a + [[b + c]] + d
+// +
+// / \
+// N-> + d
+// / \
+// X->+ c <- End
+// / \
+// a b <- Start
+// It's critical that the operator marked X is unselected, otherwise the
+// correct start point would be X itself.
+const SelectionTree::Node *getEndpoint(const SelectionTree::Node &N,
+ BinaryOperatorKind OuterOp, bool IsStart,
+ const SourceManager &SM) {
+ // If N is not a binary operator of the desired type, we have to stop here.
+ ParsedBinaryOperator Op;
+ if (!Op.parse(N.ignoreImplicit()) || Op.Kind != OuterOp)
+ return &N;
+
+ // Don't support crossing macro boundaries, that way lies madness.
+ auto FileOf = [&](const SelectionTree::Node N) {
+ return SM.getFileID(N.ASTNode.getSourceRange().getBegin());
+ };
+ FileID F = FileOf(N);
+ for (const SelectionTree::Node* Child : Op.SelectedOperands)
+ if (FileOf(*Child) != F)
+ return &N;
+
+ // Assume IsStart is true, so we want the first chunk of the subexpr.
+ // Because we're in a subtree of a Op selection, and we've disallowed
+ // crossing macro boundaries, the selection is a suffix of N.
+ // this first chunk must also be selected.
+ //
+ // The cases:
+ // 1. Operator is selected. (Implies RHS is completely selected)
+ // 1a): RHS is selected but LHS is not. Return RHS.
+ // 1b): RHS and part of LHS is selected. Recurse into LHS to find start.
+ // 2. Operator is not selected. (Implies LHS is not selected)
+ // 2a): RHS is not selected: impossible, as selection would be empty.
+ // 2b): Some of RHS is selected. Recurse into RHS to find start.
+ //
+ // Because only the selected parts are in the SelectionTree, it's tricky to
+ // identify RHS/LHS (there can be 0/1/2 children).
+ // However knowing the selection is a suffix eliminates enough cases to make
+ // it possible.
+ //
+ // (If IsStart is false, we reverse LHS/RHS everywhere)
+ assert(!Op.SelectedOperands.empty() && "got only operator on one side!");
+ assert(Op.SelectedOperands.size() <= 2 && "Funny-shaped binary operator!");
+ if (Op.OperatorSelected) {
+ if (Op.SelectedOperands.size() == 1) // Case 1a
+ return Op.SelectedOperands.front();
+ // Case 1b
+ return getEndpoint(IsStart ? *Op.SelectedOperands.front()
+ : *Op.SelectedOperands.back(),
+ OuterOp, IsStart, SM);
+ } else {
+ assert(Op.SelectedOperands.size() == 1 &&
+ "selected both sides but not operator!");
+ return getEndpoint(*Op.SelectedOperands.front(), OuterOp, IsStart, SM);
+ }
+}
+
+SourceRange ExtractionContext::getExtractionChars() const {
+ ParsedBinaryOperator Op;
+ // Special case: we're extracting an associative binary subexpression.
+ if (Op.parse(*ExprNode) && Op.associative() &&
+ Op.SelectedOperands.size() == 2) {
+ const SelectionTree::Node *LHS =
+ getEndpoint(*Op.SelectedOperands.front(), Op.Kind, true, SM);
+ const SelectionTree::Node *RHS =
+ getEndpoint(*Op.SelectedOperands.back(), Op.Kind, true, SM);
+ return SourceRange(toHalfOpenFileRange(SM, Ctx.getLangOpts(),
+ LHS->ASTNode.getSourceRange())
+ ->getBegin(),
+ toHalfOpenFileRange(SM, Ctx.getLangOpts(),
+ RHS->ASTNode.getSourceRange())
+ ->getEnd());
+ }
+ // 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 +390,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
