r373093 - [libTooling] Transformer: refine `SourceLocation` specified as anchor of changes.
Yitzhak Mandelbaum via cfe-commits
cfe-commits at lists.llvm.org
Fri Sep 27 08:26:04 PDT 2019
Author: ymandel
Date: Fri Sep 27 08:26:04 2019
New Revision: 373093
URL: http://llvm.org/viewvc/llvm-project?rev=373093&view=rev
Log:
[libTooling] Transformer: refine `SourceLocation` specified as anchor of changes.
Summary: Every change triggered by a rewrite rule is anchored at a particular
location in the source code. This patch refines how that location is chosen and
defines it as an explicit function so it can be shared by other Transformer
implementations.
This patch was inspired by a bug found by a clang tidy, wherein two changes were
anchored at the same location (the expansion loc of the macro) resulting in the
discarding of the second change.
Reviewers: gribozavr
Subscribers: cfe-commits
Tags: #clang
Differential Revision: https://reviews.llvm.org/D66652
Modified:
cfe/trunk/include/clang/Tooling/Refactoring/Transformer.h
cfe/trunk/lib/Tooling/Refactoring/Transformer.cpp
cfe/trunk/unittests/Tooling/TransformerTest.cpp
Modified: cfe/trunk/include/clang/Tooling/Refactoring/Transformer.h
URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Tooling/Refactoring/Transformer.h?rev=373093&r1=373092&r2=373093&view=diff
==============================================================================
--- cfe/trunk/include/clang/Tooling/Refactoring/Transformer.h (original)
+++ cfe/trunk/include/clang/Tooling/Refactoring/Transformer.h Fri Sep 27 08:26:04 2019
@@ -251,6 +251,12 @@ ast_matchers::internal::DynTypedMatcher
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 &
Modified: cfe/trunk/lib/Tooling/Refactoring/Transformer.cpp
URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Tooling/Refactoring/Transformer.cpp?rev=373093&r1=373092&r2=373093&view=diff
==============================================================================
--- cfe/trunk/lib/Tooling/Refactoring/Transformer.cpp (original)
+++ cfe/trunk/lib/Tooling/Refactoring/Transformer.cpp Fri Sep 27 08:26:04 2019
@@ -150,6 +150,21 @@ DynTypedMatcher tooling::detail::buildMa
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 @@ void Transformer::run(const MatchResult
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 @@ void Transformer::run(const MatchResult
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));
Modified: cfe/trunk/unittests/Tooling/TransformerTest.cpp
URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/unittests/Tooling/TransformerTest.cpp?rev=373093&r1=373092&r2=373093&view=diff
==============================================================================
--- cfe/trunk/unittests/Tooling/TransformerTest.cpp (original)
+++ cfe/trunk/unittests/Tooling/TransformerTest.cpp Fri Sep 27 08:26:04 2019
@@ -710,6 +710,57 @@ TEST_F(TransformerTest, IdentityMacro) {
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
More information about the cfe-commits
mailing list