ymandel created this revision.
ymandel added a reviewer: ilya-biryukov.
Herald added a project: clang.
ymandel updated this revision to Diff 200310.
ymandel added a comment.
reordered some declarations.
Transformer provides an enum to indicate the range of source text to be edited.
That support is now redundant with the new (and more general) RangeSelector
library, so we remove the custom enum support in favor of supporting any
RangeSelector.
Repository:
rG LLVM Github Monorepo
https://reviews.llvm.org/D62149
Files:
clang/include/clang/Tooling/Refactoring/Transformer.h
clang/lib/Tooling/Refactoring/Transformer.cpp
clang/unittests/Tooling/TransformerTest.cpp
Index: clang/unittests/Tooling/TransformerTest.cpp
===================================================================
--- clang/unittests/Tooling/TransformerTest.cpp
+++ clang/unittests/Tooling/TransformerTest.cpp
@@ -7,8 +7,8 @@
//===----------------------------------------------------------------------===//
#include "clang/Tooling/Refactoring/Transformer.h"
-
#include "clang/ASTMatchers/ASTMatchers.h"
+#include "clang/Tooling/Refactoring/RangeSelector.h"
#include "clang/Tooling/Tooling.h"
#include "llvm/Support/Errc.h"
#include "llvm/Support/Error.h"
@@ -147,7 +147,7 @@
on(expr(hasType(isOrPointsTo(StringType)))
.bind(StringExpr)),
callee(cxxMethodDecl(hasName("c_str")))))),
- change<clang::Expr>("REPLACED"));
+ change("REPLACED"));
R.Cases[0].Explanation = text("Use size() method directly on string.");
return R;
}
@@ -183,7 +183,7 @@
hasName("proto::ProtoCommandLineFlag"))))
.bind(Flag)),
unless(callee(cxxMethodDecl(hasName("GetProto"))))),
- change<clang::Expr>(Flag, "EXPR"));
+ change(Flag, "EXPR"));
std::string Input = R"cc(
proto::ProtoCommandLineFlag flag;
@@ -201,9 +201,8 @@
TEST_F(TransformerTest, NodePartNameNamedDecl) {
StringRef Fun = "fun";
- RewriteRule Rule =
- makeRule(functionDecl(hasName("bad")).bind(Fun),
- change<clang::FunctionDecl>(Fun, NodePart::Name, "good"));
+ RewriteRule Rule = makeRule(functionDecl(hasName("bad")).bind(Fun),
+ change(name(Fun), "good"));
std::string Input = R"cc(
int bad(int x);
@@ -235,7 +234,7 @@
StringRef Ref = "ref";
testRule(makeRule(declRefExpr(to(functionDecl(hasName("bad")))).bind(Ref),
- change<clang::Expr>(Ref, NodePart::Name, "good")),
+ change(name(Ref), "good")),
Input, Expected);
}
@@ -253,7 +252,7 @@
StringRef Ref = "ref";
Transformer T(makeRule(declRefExpr(to(functionDecl())).bind(Ref),
- change<clang::Expr>(Ref, NodePart::Name, "good")),
+ change(name(Ref), "good")),
consumer());
T.registerMatchers(&MatchFinder);
EXPECT_FALSE(rewrite(Input));
@@ -262,7 +261,7 @@
TEST_F(TransformerTest, NodePartMember) {
StringRef E = "expr";
RewriteRule Rule = makeRule(memberExpr(member(hasName("bad"))).bind(E),
- change<clang::Expr>(E, NodePart::Member, "good"));
+ change(member(E), "good"));
std::string Input = R"cc(
struct S {
@@ -315,9 +314,8 @@
)cc";
StringRef E = "expr";
- testRule(makeRule(memberExpr().bind(E),
- change<clang::Expr>(E, NodePart::Member, "good")),
- Input, Expected);
+ testRule(makeRule(memberExpr().bind(E), change(member(E), "good")), Input,
+ Expected);
}
TEST_F(TransformerTest, NodePartMemberMultiToken) {
@@ -347,9 +345,9 @@
)cc";
StringRef MemExpr = "member";
- testRule(makeRule(memberExpr().bind(MemExpr),
- change<clang::Expr>(MemExpr, NodePart::Member, "good")),
- Input, Expected);
+ testRule(
+ makeRule(memberExpr().bind(MemExpr), change(member(MemExpr), "good")),
+ Input, Expected);
}
TEST_F(TransformerTest, MultiChange) {
@@ -371,8 +369,8 @@
StringRef C = "C", T = "T", E = "E";
testRule(makeRule(ifStmt(hasCondition(expr().bind(C)),
hasThen(stmt().bind(T)), hasElse(stmt().bind(E))),
- {change<Expr>(C, "true"), change<Stmt>(T, "{ /* then */ }"),
- change<Stmt>(E, "{ /* else */ }")}),
+ {change(C, "true"), change(statement(T), "{ /* then */ }"),
+ change(statement(E), "{ /* else */ }")}),
Input, Expected);
}
@@ -383,7 +381,7 @@
hasName("proto::ProtoCommandLineFlag"))))
.bind(Flag)),
unless(callee(cxxMethodDecl(hasName("GetProto"))))),
- change<clang::Expr>(Flag, "PROTO"));
+ change(Flag, "PROTO"));
std::string Input = R"cc(
proto::ProtoCommandLineFlag flag;
@@ -410,7 +408,7 @@
hasArgument(0, cxxMemberCallExpr(
on(expr().bind(S)),
callee(cxxMethodDecl(hasName("c_str")))))),
- change<clang::Expr>("DISTINCT"));
+ change("DISTINCT"));
}
TEST_F(TransformerTest, OrderedRuleRelated) {
@@ -475,7 +473,7 @@
-> llvm::Expected<std::string> {
return llvm::createStringError(llvm::errc::invalid_argument, "ERROR");
};
- Transformer T(makeRule(binaryOperator().bind(O), change<Expr>(O, AlwaysFail)),
+ Transformer T(makeRule(binaryOperator().bind(O), change(O, AlwaysFail)),
consumer());
T.registerMatchers(&MatchFinder);
EXPECT_FALSE(rewrite(Input));
@@ -488,10 +486,9 @@
std::string Input = "int conflictOneRule() { return 3 + 7; }";
// Try to change the whole binary-operator expression AND one its operands:
StringRef O = "O", L = "L";
- Transformer T(
- makeRule(binaryOperator(hasLHS(expr().bind(L))).bind(O),
- {change<Expr>(O, "DELETE_OP"), change<Expr>(L, "DELETE_LHS")}),
- consumer());
+ Transformer T(makeRule(binaryOperator(hasLHS(expr().bind(L))).bind(O),
+ {change(O, "DELETE_OP"), change(L, "DELETE_LHS")}),
+ consumer());
T.registerMatchers(&MatchFinder);
EXPECT_FALSE(rewrite(Input));
EXPECT_THAT(Changes, IsEmpty());
@@ -503,8 +500,7 @@
std::string Input = "int conflictOneRule() { return -7; }";
// Try to change the whole binary-operator expression AND one its operands:
StringRef E = "E";
- Transformer T(makeRule(expr().bind(E), change<Expr>(E, "DELETE_EXPR")),
- consumer());
+ Transformer T(makeRule(expr().bind(E), change(E, "DELETE_EXPR")), consumer());
T.registerMatchers(&MatchFinder);
// The rewrite process fails because the changes conflict with each other...
EXPECT_FALSE(rewrite(Input));
@@ -516,9 +512,9 @@
TEST_F(TransformerTest, ErrorOccurredMatchSkipped) {
// Syntax error in the function body:
std::string Input = "void errorOccurred() { 3 }";
- Transformer T(makeRule(functionDecl(hasName("errorOccurred")),
- change<Decl>("DELETED;")),
- consumer());
+ Transformer T(
+ makeRule(functionDecl(hasName("errorOccurred")), change("DELETED;")),
+ consumer());
T.registerMatchers(&MatchFinder);
// The rewrite process itself fails...
EXPECT_FALSE(rewrite(Input));
Index: clang/lib/Tooling/Refactoring/Transformer.cpp
===================================================================
--- clang/lib/Tooling/Refactoring/Transformer.cpp
+++ clang/lib/Tooling/Refactoring/Transformer.cpp
@@ -32,11 +32,7 @@
using ast_type_traits::ASTNodeKind;
using ast_type_traits::DynTypedNode;
using llvm::Error;
-using llvm::Expected;
-using llvm::Optional;
using llvm::StringError;
-using llvm::StringRef;
-using llvm::Twine;
using MatchResult = MatchFinder::MatchResult;
@@ -71,91 +67,12 @@
return false;
}
-static llvm::Error invalidArgumentError(Twine Message) {
- return llvm::make_error<StringError>(llvm::errc::invalid_argument, Message);
-}
-
-static llvm::Error typeError(StringRef Id, const ASTNodeKind &Kind,
- Twine Message) {
- return invalidArgumentError(
- Message + " (node id=" + Id + " kind=" + Kind.asStringRef() + ")");
-}
-
-static llvm::Error missingPropertyError(StringRef Id, Twine Description,
- StringRef Property) {
- return invalidArgumentError(Description + " requires property '" + Property +
- "' (node id=" + Id + ")");
-}
-
-static Expected<CharSourceRange>
-getTargetRange(StringRef Target, const DynTypedNode &Node, ASTNodeKind Kind,
- NodePart TargetPart, ASTContext &Context) {
- switch (TargetPart) {
- case NodePart::Node: {
- // For non-expression statements, associate any trailing semicolon with the
- // statement text. However, if the target was intended as an expression (as
- // indicated by its kind) then we do not associate any trailing semicolon
- // with it. We only associate the exact expression text.
- if (Node.get<Stmt>() != nullptr) {
- auto ExprKind = ASTNodeKind::getFromNodeKind<clang::Expr>();
- if (!ExprKind.isBaseOf(Kind))
- return getExtendedRange(Node, tok::TokenKind::semi, Context);
- }
- return CharSourceRange::getTokenRange(Node.getSourceRange());
- }
- case NodePart::Member:
- if (auto *M = Node.get<clang::MemberExpr>())
- return CharSourceRange::getTokenRange(
- M->getMemberNameInfo().getSourceRange());
- return typeError(Target, Node.getNodeKind(),
- "NodePart::Member applied to non-MemberExpr");
- case NodePart::Name:
- if (const auto *D = Node.get<clang::NamedDecl>()) {
- if (!D->getDeclName().isIdentifier())
- return missingPropertyError(Target, "NodePart::Name", "identifier");
- SourceLocation L = D->getLocation();
- auto R = CharSourceRange::getTokenRange(L, L);
- // Verify that the range covers exactly the name.
- // FIXME: extend this code to support cases like `operator +` or
- // `foo<int>` for which this range will be too short. Doing so will
- // require subcasing `NamedDecl`, because it doesn't provide virtual
- // access to the \c DeclarationNameInfo.
- if (getText(R, Context) != D->getName())
- return CharSourceRange();
- return R;
- }
- if (const auto *E = Node.get<clang::DeclRefExpr>()) {
- if (!E->getNameInfo().getName().isIdentifier())
- return missingPropertyError(Target, "NodePart::Name", "identifier");
- SourceLocation L = E->getLocation();
- return CharSourceRange::getTokenRange(L, L);
- }
- if (const auto *I = Node.get<clang::CXXCtorInitializer>()) {
- if (!I->isMemberInitializer() && I->isWritten())
- return missingPropertyError(Target, "NodePart::Name",
- "explicit member initializer");
- SourceLocation L = I->getMemberLocation();
- return CharSourceRange::getTokenRange(L, L);
- }
- return typeError(
- Target, Node.getNodeKind(),
- "NodePart::Name applied to neither DeclRefExpr, NamedDecl nor "
- "CXXCtorInitializer");
- }
- llvm_unreachable("Unexpected case in NodePart type.");
-}
-
Expected<SmallVector<tooling::detail::Transformation, 1>>
tooling::detail::translateEdits(const MatchResult &Result,
llvm::ArrayRef<ASTEdit> Edits) {
- SmallVector<Transformation, 1> Transformations;
- auto &NodesMap = Result.Nodes.getMap();
+ SmallVector<tooling::detail::Transformation, 1> Transformations;
for (const auto &Edit : Edits) {
- auto It = NodesMap.find(Edit.Target);
- assert(It != NodesMap.end() && "Edit target must be bound in the match.");
-
- Expected<CharSourceRange> Range = getTargetRange(
- Edit.Target, It->second, Edit.Kind, Edit.Part, *Result.Context);
+ Expected<CharSourceRange> Range = Edit.TargetRange(Result);
if (!Range)
return Range.takeError();
if (Range->isInvalid() ||
@@ -164,7 +81,7 @@
auto Replacement = Edit.Replacement(Result);
if (!Replacement)
return Replacement.takeError();
- Transformation T;
+ tooling::detail::Transformation T;
T.Range = *Range;
T.Replacement = std::move(*Replacement);
Transformations.push_back(std::move(T));
@@ -172,6 +89,13 @@
return Transformations;
}
+ASTEdit tooling::change(RangeSelector S, TextGenerator Replacement) {
+ ASTEdit E;
+ E.TargetRange = std::move(S);
+ E.Replacement = std::move(Replacement);
+ return E;
+}
+
RewriteRule tooling::makeRule(DynTypedMatcher M,
SmallVector<ASTEdit, 1> Edits) {
return RewriteRule{
@@ -255,7 +179,7 @@
DynTypedMatcher M = joinCaseMatchers(Rule);
M.setAllowBind(true);
// `tryBind` is guaranteed to succeed, because `AllowBind` was set to true.
- return *M.tryBind(RewriteRule::RootId);
+ return *M.tryBind(RewriteRule::RootID);
}
// Finds the case that was "selected" -- that is, whose matcher triggered the
@@ -275,7 +199,7 @@
llvm_unreachable("No tag found for this rule.");
}
-constexpr llvm::StringLiteral RewriteRule::RootId;
+constexpr llvm::StringLiteral RewriteRule::RootID;
void Transformer::registerMatchers(MatchFinder *MatchFinder) {
MatchFinder->addDynamicMatcher(tooling::detail::buildMatcher(Rule), this);
@@ -287,7 +211,7 @@
// Verify the existence and validity of the AST node that roots this rule.
auto &NodesMap = Result.Nodes.getMap();
- auto Root = NodesMap.find(RewriteRule::RootId);
+ auto Root = NodesMap.find(RewriteRule::RootID);
assert(Root != NodesMap.end() && "Transformation failed: missing root node.");
SourceLocation RootLoc = Result.SourceManager->getExpansionLoc(
Root->second.getSourceRange().getBegin());
Index: clang/include/clang/Tooling/Refactoring/Transformer.h
===================================================================
--- clang/include/clang/Tooling/Refactoring/Transformer.h
+++ clang/include/clang/Tooling/Refactoring/Transformer.h
@@ -19,6 +19,7 @@
#include "clang/ASTMatchers/ASTMatchers.h"
#include "clang/ASTMatchers/ASTMatchersInternal.h"
#include "clang/Tooling/Refactoring/AtomicChange.h"
+#include "clang/Tooling/Refactoring/RangeSelector.h"
#include "llvm/ADT/STLExtras.h"
#include "llvm/ADT/SmallVector.h"
#include "llvm/Support/Error.h"
@@ -30,19 +31,6 @@
namespace clang {
namespace tooling {
-/// Determines the part of the AST node to replace. We support this to work
-/// around the fact that the AST does not differentiate various syntactic
-/// elements into their own nodes, so users can specify them relative to a node,
-/// instead.
-enum class NodePart {
- /// The node itself.
- Node,
- /// Given a \c MemberExpr, selects the member's token.
- Member,
- /// Given a \c NamedDecl or \c CxxCtorInitializer, selects that token of the
- /// relevant name, not including qualifiers.
- Name,
-};
// Note that \p TextGenerator is allowed to fail, e.g. when trying to access a
// matched node that was not bound. Allowing this to fail simplifies error
@@ -93,56 +81,11 @@
// change<Expr>("different_expr")
// \endcode
struct ASTEdit {
- // The (bound) id of the node whose source will be replaced. This id should
- // never be the empty string.
- std::string Target;
- ast_type_traits::ASTNodeKind Kind;
- NodePart Part;
+ RangeSelector TargetRange;
TextGenerator Replacement;
TextGenerator Note;
};
-// Convenience functions for creating \c ASTEdits. They all must be explicitly
-// instantiated with the desired AST type. Each overload includes both \c
-// std::string and \c TextGenerator versions.
-
-// FIXME: For overloads taking a \c NodePart, constrain the valid values of \c
-// Part based on the type \c T.
-template <typename T>
-ASTEdit change(StringRef Target, NodePart Part, TextGenerator Replacement) {
- ASTEdit E;
- E.Target = Target.str();
- E.Kind = ast_type_traits::ASTNodeKind::getFromNodeKind<T>();
- E.Part = Part;
- E.Replacement = std::move(Replacement);
- return E;
-}
-
-template <typename T>
-ASTEdit change(StringRef Target, NodePart Part, std::string Replacement) {
- return change<T>(Target, Part, text(std::move(Replacement)));
-}
-
-/// Variant of \c change for which the NodePart defaults to the whole node.
-template <typename T>
-ASTEdit change(StringRef Target, TextGenerator Replacement) {
- return change<T>(Target, NodePart::Node, std::move(Replacement));
-}
-
-/// Variant of \c change for which the NodePart defaults to the whole node.
-template <typename T>
-ASTEdit change(StringRef Target, std::string Replacement) {
- return change<T>(Target, text(std::move(Replacement)));
-}
-
-/// Variant of \c change that selects the node of the entire match.
-template <typename T> ASTEdit change(TextGenerator Replacement);
-
-/// Variant of \c change that selects the node of the entire match.
-template <typename T> ASTEdit change(std::string Replacement) {
- return change<T>(text(std::move(Replacement)));
-}
-
/// Description of a source-code transformation.
//
// A *rewrite rule* describes a transformation of source code. A simple rule
@@ -175,9 +118,9 @@
// We expect RewriteRules will most commonly include only one case.
SmallVector<Case, 1> Cases;
- // Id used as the default target of each match. The node described by the
+ // ID used as the default target of each match. The node described by the
// matcher is should always be bound to this id.
- static constexpr llvm::StringLiteral RootId = "___root___";
+ static constexpr llvm::StringLiteral RootID = "___root___";
};
/// Convenience function for constructing a simple \c RewriteRule.
@@ -235,10 +178,28 @@
// ```
RewriteRule applyFirst(ArrayRef<RewriteRule> Rules);
-// Define this overload of `change` here because RewriteRule::RootId is not in
-// scope at the declaration point above.
-template <typename T> ASTEdit change(TextGenerator Replacement) {
- return change<T>(RewriteRule::RootId, NodePart::Node, std::move(Replacement));
+/// Replaces a portion of the source text with \p Replacement.
+ASTEdit change(RangeSelector Target, TextGenerator Replacement);
+
+/// These overloads of \c change allow the user to pass a string-type argument
+/// where a \c TextGenerator, \c RangeSelector are otherwise expected.
+inline ASTEdit change(RangeSelector Target, std::string Replacement) {
+ return change(std::move(Target), text(std::move(Replacement)));
+}
+inline ASTEdit change(StringRef Target, TextGenerator Replacement) {
+ return change(node(Target), std::move(Replacement));
+}
+inline ASTEdit change(StringRef Target, std::string Replacement) {
+ return change(node(Target), text(Replacement));
+}
+
+/// These overloads implicitly identify the matched portion of a RewriteRule as
+/// the target of the change.
+inline ASTEdit change(std::string Replacement) {
+ return change(RewriteRule::RootID, std::move(Replacement));
+}
+inline ASTEdit change(TextGenerator Replacement) {
+ return change(RewriteRule::RootID, std::move(Replacement));
}
/// The following three functions are a low-level part of the RewriteRule
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
