ymandel updated this revision to Diff 222177.
ymandel added a comment.
Herald added a subscriber: jfb.
reworked to follow same scheme as clang-tidy version, per discussion.
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,14 +193,6 @@
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());
- assert(RootLoc.isValid() && "Invalid location for Root node of match.");
-
RewriteRule::Case Case = tooling::detail::findSelectedCase(Result, Rule);
auto Transformations = tooling::detail::translateEdits(Result, Case.Edits);
if (!Transformations) {
@@ -195,14 +202,16 @@
if (Transformations->empty()) {
// No rewrite applied (but no error encountered either).
- RootLoc.print(llvm::errs() << "note: skipping match at loc ",
- *Result.SourceManager);
+ detail::getRuleMatchLoc(Result).print(
+ llvm::errs() << "note: skipping match at loc ", *Result.SourceManager);
llvm::errs() << "\n";
return;
}
- // Record the results in the AtomicChange.
- AtomicChange AC(*Result.SourceManager, RootLoc);
+ // Record the results in the AtomicChange, anchored at the location of the
+ // first change.
+ AtomicChange AC(*Result.SourceManager,
+ (*Transformations)[0].Range.getBegin());
for (const auto &T : *Transformations) {
if (auto Err = AC.replace(*Result.SourceManager, T.Range, T.Replacement)) {
Consumer(std::move(Err));
Index: clang/include/clang/Tooling/Refactoring/Transformer.h
===================================================================
--- clang/include/clang/Tooling/Refactoring/Transformer.h
+++ clang/include/clang/Tooling/Refactoring/Transformer.h
@@ -251,6 +251,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
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
