ymandel updated this revision to Diff 216921.
ymandel marked 2 inline comments as done.
ymandel added a comment.
comments tweaks.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D66652/new/
https://reviews.llvm.org/D66652
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
@@ -710,6 +710,57 @@
testRule(ruleStrlenSize(), Input, Expected);
}
+// Tests that two changes in a single macro expansion do not lead to conflicts
+// in applying the changes.
+TEST_F(TransformerTest, TwoChangesInOneMacroExpansion) {
+ std::string Input = R"cc(
+#define PLUS(a,b) (a) + (b)
+ int f() { return PLUS(3, 4); }
+ )cc";
+ std::string Expected = R"cc(
+#define PLUS(a,b) (a) + (b)
+ int f() { return PLUS(LIT, LIT); }
+ )cc";
+
+ testRule(makeRule(integerLiteral(), change(text("LIT"))), Input, Expected);
+}
+
+// Tests case where the rule's match spans both source from the macro and its
+// arg, with the begin location (the "anchor") being the arg.
+TEST_F(TransformerTest, MatchSpansMacroTextButChangeDoesNot) {
+ std::string Input = R"cc(
+#define PLUS_ONE(a) a + 1
+ int f() { return PLUS_ONE(3); }
+ )cc";
+ std::string Expected = R"cc(
+#define PLUS_ONE(a) a + 1
+ int f() { return PLUS_ONE(LIT); }
+ )cc";
+
+ StringRef E = "expr";
+ testRule(makeRule(binaryOperator(hasLHS(expr().bind(E))),
+ change(node(E), text("LIT"))),
+ Input, Expected);
+}
+
+// Tests case where the rule's match spans both source from the macro and its
+// arg, with the begin location (the "anchor") being inside the macro.
+TEST_F(TransformerTest, MatchSpansMacroTextButChangeDoesNotAnchoredInMacro) {
+ std::string Input = R"cc(
+#define PLUS_ONE(a) 1 + a
+ int f() { return PLUS_ONE(3); }
+ )cc";
+ std::string Expected = R"cc(
+#define PLUS_ONE(a) 1 + a
+ int f() { return PLUS_ONE(LIT); }
+ )cc";
+
+ StringRef E = "expr";
+ testRule(makeRule(binaryOperator(hasRHS(expr().bind(E))),
+ change(node(E), text("LIT"))),
+ Input, Expected);
+}
+
// No rewrite is applied when the changed text does not encompass the entirety
// of the expanded text. That is, the edit would have to be applied to the
// macro's definition to succeed and editing the expansion point would not
Index: clang/lib/Tooling/Refactoring/Transformer.cpp
===================================================================
--- clang/lib/Tooling/Refactoring/Transformer.cpp
+++ clang/lib/Tooling/Refactoring/Transformer.cpp
@@ -150,6 +150,21 @@
return Ms[0];
}
+SourceLocation tooling::detail::getRuleMatchLoc(const MatchResult &Result) {
+ auto &NodesMap = Result.Nodes.getMap();
+ auto Root = NodesMap.find(RewriteRule::RootID);
+ assert(Root != NodesMap.end() && "Transformation failed: missing root node.");
+ llvm::Optional<CharSourceRange> RootRange = getRangeForEdit(
+ CharSourceRange::getTokenRange(Root->second.getSourceRange()),
+ *Result.Context);
+ if (RootRange)
+ return RootRange->getBegin();
+ // The match doesn't have a coherent range, so fall back to the expansion
+ // location as the "beginning" of the match.
+ return Result.SourceManager->getExpansionLoc(
+ Root->second.getSourceRange().getBegin());
+}
+
// Finds the case that was "selected" -- that is, whose matcher triggered the
// `MatchResult`.
const RewriteRule::Case &
@@ -178,12 +193,7 @@
if (Result.Context->getDiagnostics().hasErrorOccurred())
return;
- // Verify the existence and validity of the AST node that roots this rule.
- auto &NodesMap = Result.Nodes.getMap();
- auto Root = NodesMap.find(RewriteRule::RootID);
- assert(Root != NodesMap.end() && "Transformation failed: missing root node.");
- SourceLocation RootLoc = Result.SourceManager->getExpansionLoc(
- Root->second.getSourceRange().getBegin());
+ SourceLocation RootLoc = tooling::detail::getRuleMatchLoc(Result);
assert(RootLoc.isValid() && "Invalid location for Root node of match.");
RewriteRule::Case Case = tooling::detail::findSelectedCase(Result, Rule);
Index: clang/include/clang/Tooling/Refactoring/Transformer.h
===================================================================
--- clang/include/clang/Tooling/Refactoring/Transformer.h
+++ clang/include/clang/Tooling/Refactoring/Transformer.h
@@ -254,6 +254,12 @@
std::vector<ast_matchers::internal::DynTypedMatcher>
buildMatchers(const RewriteRule &Rule);
+/// Gets the beginning location of the source matched by a rewrite rule. If the
+/// match occurs within a macro expansion, returns the beginning of the
+/// expansion point. `Result` must come from the matching of a rewrite rule.
+SourceLocation
+getRuleMatchLoc(const ast_matchers::MatchFinder::MatchResult &Result);
+
/// 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
