ymandel updated this revision to Diff 214442.
ymandel marked 14 inline comments as done.
ymandel added a comment.
Rewrote the code and tests.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D65877/new/
https://reviews.llvm.org/D65877
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
@@ -482,65 +482,85 @@
testRule(applyFirst({ruleStrlenSize(), FlagRule}), Input, Expected);
}
-// Version of ruleStrlenSizeAny that inserts a method with a different name than
-// ruleStrlenSize, so we can tell their effect apart.
-RewriteRule ruleStrlenSizeDistinct() {
- StringRef S;
- return makeRule(
- callExpr(callee(functionDecl(hasName("strlen"))),
- hasArgument(0, cxxMemberCallExpr(
- on(expr().bind(S)),
- callee(cxxMethodDecl(hasName("c_str")))))),
- change(text("DISTINCT")));
-}
-
TEST_F(TransformerTest, OrderedRuleRelated) {
std::string Input = R"cc(
- namespace foo {
- struct mystring {
- char* c_str();
- };
- int f(mystring s) { return strlen(s.c_str()); }
- } // namespace foo
- int g(string s) { return strlen(s.c_str()); }
+ void f1();
+ void f2();
+ void call_f1() { f1(); }
+ void call_f2() { f2(); }
)cc";
std::string Expected = R"cc(
- namespace foo {
- struct mystring {
- char* c_str();
- };
- int f(mystring s) { return DISTINCT; }
- } // namespace foo
- int g(string s) { return REPLACED; }
+ void f1();
+ void f2();
+ void call_f1() { REPLACE_F1; }
+ void call_f2() { REPLACE_F1_OR_F2; }
)cc";
- testRule(applyFirst({ruleStrlenSize(), ruleStrlenSizeDistinct()}), Input,
- Expected);
+ RewriteRule ReplaceF1 =
+ makeRule(callExpr(callee(functionDecl(hasName("f1")))),
+ change(text("REPLACE_F1")));
+ RewriteRule ReplaceF1OrF2 =
+ makeRule(callExpr(callee(functionDecl(hasAnyName("f1", "f2")))),
+ change(text("REPLACE_F1_OR_F2")));
+ testRule(applyFirst({ReplaceF1, ReplaceF1OrF2}), Input, Expected);
}
-// Change the order of the rules to get a different result.
+// Change the order of the rules to get a different result. When `ReplaceF1OrF2`
+// comes first, it applies for both uses, so `ReplaceF1` never applies.
TEST_F(TransformerTest, OrderedRuleRelatedSwapped) {
std::string Input = R"cc(
- namespace foo {
- struct mystring {
- char* c_str();
- };
- int f(mystring s) { return strlen(s.c_str()); }
- } // namespace foo
- int g(string s) { return strlen(s.c_str()); }
+ void f1();
+ void f2();
+ void call_f1() { f1(); }
+ void call_f2() { f2(); }
)cc";
std::string Expected = R"cc(
- namespace foo {
- struct mystring {
- char* c_str();
- };
- int f(mystring s) { return DISTINCT; }
- } // namespace foo
- int g(string s) { return DISTINCT; }
+ void f1();
+ void f2();
+ void call_f1() { REPLACE_F1_OR_F2; }
+ void call_f2() { REPLACE_F1_OR_F2; }
+ )cc";
+
+ RewriteRule ReplaceF1 =
+ makeRule(callExpr(callee(functionDecl(hasName("f1")))),
+ change(text("REPLACE_F1")));
+ RewriteRule ReplaceF1OrF2 =
+ makeRule(callExpr(callee(functionDecl(hasAnyName("f1", "f2")))),
+ change(text("REPLACE_F1_OR_F2")));
+ testRule(applyFirst({ReplaceF1OrF2, ReplaceF1}), Input, Expected);
+}
+
+// Verify that a set of rules whose matchers have different base kinds works
+// properly, including that `applyFirst` produces multiple matchers. We test
+// two different kinds of rules: Expr and Decl. We place the Decl rule in the
+// middle to test that `buildMatchers` works even when the kinds aren't grouped
+// together.
+TEST_F(TransformerTest, OrderedRuleMultipleKinds) {
+ std::string Input = R"cc(
+ void f1();
+ void f2();
+ void call_f1() { f1(); }
+ void call_f2() { f2(); }
)cc";
-
- testRule(applyFirst({ruleStrlenSizeDistinct(), ruleStrlenSize()}), Input,
- Expected);
+ std::string Expected = R"cc(
+ void f1();
+ void DECL_RULE();
+ void call_f1() { REPLACE_F1; }
+ void call_f2() { REPLACE_F1_OR_F2; }
+ )cc";
+
+ RewriteRule ReplaceF1 =
+ makeRule(callExpr(callee(functionDecl(hasName("f1")))),
+ change(text("REPLACE_F1")));
+ RewriteRule ReplaceF1OrF2 =
+ makeRule(callExpr(callee(functionDecl(hasAnyName("f1", "f2")))),
+ change(text("REPLACE_F1_OR_F2")));
+ RewriteRule DeclRule = makeRule(functionDecl(hasName("f2")).bind("fun"),
+ change(name("fun"), text("DECL_RULE")));
+
+ RewriteRule Rule = applyFirst({ReplaceF1, DeclRule, ReplaceF1OrF2});
+ EXPECT_EQ(tooling::detail::buildMatchers(Rule).size(), 2UL);
+ testRule(Rule, Input, Expected);
}
//
Index: clang/lib/Tooling/Refactoring/Transformer.cpp
===================================================================
--- clang/lib/Tooling/Refactoring/Transformer.cpp
+++ clang/lib/Tooling/Refactoring/Transformer.cpp
@@ -80,52 +80,28 @@
Case.AddedIncludes.emplace_back(Header.str(), Format);
}
-// Determines whether A is a base type of B in the class hierarchy, including
-// the implicit relationship of Type and QualType.
-static bool isBaseOf(ASTNodeKind A, ASTNodeKind B) {
+// Gets the kind corresponding to the given matcher for assigning it to bucket.
+// Requires that the `M` is a node matcher.
+static ASTNodeKind getKind(const DynTypedMatcher &M) {
+ ASTNodeKind K = M.getSupportedKind();
static auto TypeKind = ASTNodeKind::getFromNodeKind<Type>();
static auto QualKind = ASTNodeKind::getFromNodeKind<QualType>();
- /// Mimic the implicit conversions of Matcher<>.
- /// - From Matcher<Type> to Matcher<QualType>
- /// - From Matcher<Base> to Matcher<Derived>
- return (A.isSame(TypeKind) && B.isSame(QualKind)) || A.isBaseOf(B);
-}
-
-// Try to find a common kind to which all of the rule's matchers can be
-// converted.
-static ASTNodeKind
-findCommonKind(const SmallVectorImpl<RewriteRule::Case> &Cases) {
- assert(!Cases.empty() && "Rule must have at least one case.");
- ASTNodeKind JoinKind = Cases[0].Matcher.getSupportedKind();
- // Find a (least) Kind K, for which M.canConvertTo(K) holds, for all matchers
- // M in Rules.
- for (const auto &Case : Cases) {
- auto K = Case.Matcher.getSupportedKind();
- if (isBaseOf(JoinKind, K)) {
- JoinKind = K;
- continue;
- }
- if (K.isSame(JoinKind) || isBaseOf(K, JoinKind))
- // JoinKind is already the lowest.
- continue;
- // K and JoinKind are unrelated -- there is no least common kind.
- return ASTNodeKind();
- }
- return JoinKind;
+ // Type and QualType can be put together, but otherwise each root type is
+ // distinct.
+ if (K.isSame(TypeKind)) return QualKind;
+ return K;
}
// Binds each rule's matcher to a unique (and deterministic) tag based on
-// `TagBase`.
-static std::vector<DynTypedMatcher>
-taggedMatchers(StringRef TagBase,
- const SmallVectorImpl<RewriteRule::Case> &Cases) {
+// `TagBase` and the id paired with the case.
+static std::vector<DynTypedMatcher> taggedMatchers(
+ StringRef TagBase,
+ const SmallVectorImpl<std::pair<size_t, RewriteRule::Case>> &Cases) {
std::vector<DynTypedMatcher> Matchers;
Matchers.reserve(Cases.size());
- size_t count = 0;
for (const auto &Case : Cases) {
- std::string Tag = (TagBase + Twine(count)).str();
- ++count;
- auto M = Case.Matcher.tryBind(Tag);
+ std::string Tag = (TagBase + Twine(Case.first)).str();
+ auto M = Case.second.Matcher.tryBind(Tag);
assert(M && "RewriteRule matchers should be bindable.");
Matchers.push_back(*std::move(M));
}
@@ -142,22 +118,34 @@
return R;
}
-static DynTypedMatcher joinCaseMatchers(const RewriteRule &Rule) {
- assert(!Rule.Cases.empty() && "Rule must have at least one case.");
- if (Rule.Cases.size() == 1)
- return Rule.Cases[0].Matcher;
+std::vector<DynTypedMatcher>
+tooling::detail::buildMatchers(const RewriteRule &Rule) {
+ // Map the cases into buckets of matchers -- one for each "root" AST kind,
+ // which guarantees that they can be combined in a single anyOf matcher. Each
+ // case is paired with an identifying number that is converted to a string id
+ // in `taggedMatchers`.
+ std::map<ASTNodeKind, SmallVector<std::pair<size_t, RewriteRule::Case>, 1>>
+ Buckets;
+ const SmallVectorImpl<RewriteRule::Case> &Cases = Rule.Cases;
+ for (int I = 0, N = Cases.size(); I < N; ++I)
+ Buckets[getKind(Cases[I].Matcher)].emplace_back(I, Cases[I]);
- auto CommonKind = findCommonKind(Rule.Cases);
- assert(!CommonKind.isNone() && "Cases must have compatible matchers.");
- return DynTypedMatcher::constructVariadic(
- DynTypedMatcher::VO_AnyOf, CommonKind, taggedMatchers("Tag", Rule.Cases));
+ std::vector<DynTypedMatcher> Matchers;
+ for (const auto &Bucket : Buckets) {
+ DynTypedMatcher M = DynTypedMatcher::constructVariadic(
+ DynTypedMatcher::VO_AnyOf, Bucket.first,
+ taggedMatchers("Tag", Bucket.second));
+ M.setAllowBind(true);
+ // `tryBind` is guaranteed to succeed, because `AllowBind` was set to true.
+ Matchers.push_back(*M.tryBind(RewriteRule::RootID));
+ }
+ return Matchers;
}
DynTypedMatcher tooling::detail::buildMatcher(const RewriteRule &Rule) {
- DynTypedMatcher M = joinCaseMatchers(Rule);
- M.setAllowBind(true);
- // `tryBind` is guaranteed to succeed, because `AllowBind` was set to true.
- return *M.tryBind(RewriteRule::RootID);
+ std::vector<DynTypedMatcher> Ms = buildMatchers(Rule);
+ assert(Ms.size() == 1 && "Cases must have compatible matchers.");
+ return Ms[0];
}
// Finds the case that was "selected" -- that is, whose matcher triggered the
@@ -180,7 +168,8 @@
constexpr llvm::StringLiteral RewriteRule::RootID;
void Transformer::registerMatchers(MatchFinder *MatchFinder) {
- MatchFinder->addDynamicMatcher(tooling::detail::buildMatcher(Rule), this);
+ for (auto &Matcher : tooling::detail::buildMatchers(Rule))
+ MatchFinder->addDynamicMatcher(Matcher, this);
}
void Transformer::run(const MatchResult &Result) {
@@ -222,12 +211,12 @@
for (const auto &I : Case.AddedIncludes) {
auto &Header = I.first;
switch (I.second) {
- case IncludeFormat::Quoted:
- AC.addHeader(Header);
- break;
- case IncludeFormat::Angled:
- AC.addHeader((llvm::Twine("<") + Header + ">").str());
- break;
+ case IncludeFormat::Quoted:
+ AC.addHeader(Header);
+ break;
+ case IncludeFormat::Angled:
+ AC.addHeader((llvm::Twine("<") + Header + ">").str());
+ break;
}
}
Index: clang/include/clang/Tooling/Refactoring/Transformer.h
===================================================================
--- clang/include/clang/Tooling/Refactoring/Transformer.h
+++ clang/include/clang/Tooling/Refactoring/Transformer.h
@@ -160,11 +160,9 @@
void addInclude(RewriteRule &Rule, llvm::StringRef Header,
IncludeFormat Format = IncludeFormat::Quoted);
-/// Applies the first rule whose pattern matches; other rules are ignored.
-///
-/// N.B. All of the rules must use the same kind of matcher (that is, share a
-/// base class in the AST hierarchy). However, this constraint is caused by an
-/// implementation detail and should be lifted in the future.
+/// Applies the first rule whose pattern matches; other rules are ignored. If
+/// the matchers are independent then order doesn't matter. In that case,
+/// `applyFirst` is simply joining the set of rules into one.
//
// `applyFirst` is like an `anyOf` matcher with an edit action attached to each
// of its cases. Anywhere you'd use `anyOf(m1.bind("id1"), m2.bind("id2"))` and
@@ -243,8 +241,16 @@
// public and well-supported and move them out of `detail`.
namespace detail {
/// Builds a single matcher for the rule, covering all of the rule's cases.
+/// Only supports Rules whose cases' matchers all share the same base "kind"
+/// (`Stmt`, `Decl`, etc.) Deprecated: use `buildMatchers` instead, which
+/// supports mixing matchers of different kinds.
ast_matchers::internal::DynTypedMatcher buildMatcher(const RewriteRule &Rule);
+/// Builds a set of matchers that cover the rule (one for each distinct matcher
+/// base kind: Stmt, Decl, etc.)
+std::vector<ast_matchers::internal::DynTypedMatcher>
+buildMatchers(const RewriteRule &Rule);
+
/// Returns the \c Case of \c Rule that was selected in the match result.
/// Assumes a matcher built with \c buildMatcher.
const RewriteRule::Case &
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
