ymandel updated this revision to Diff 216838. ymandel added a comment. comment 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,22 @@ return Ms[0]; } +SourceLocation tooling::detail::getRuleMatchLoc(const MatchResult &Result) { + // 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."); + llvm::Optional<CharSourceRange> RootRange = getRangeForEdit( + CharSourceRange::getTokenRange(Root->second.getSourceRange()), + *Result.Context); + if (RootRange) + return RootRange->getBegin(); + // The match doens'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 & @@ -179,11 +195,7 @@ 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