r366473 - [LibTooling] Relax Transformer to allow rewriting macro expansions
Yitzhak Mandelbaum via cfe-commits
cfe-commits at lists.llvm.org
Thu Jul 18 10:44:54 PDT 2019
Author: ymandel
Date: Thu Jul 18 10:44:54 2019
New Revision: 366473
URL: http://llvm.org/viewvc/llvm-project?rev=366473&view=rev
Log:
[LibTooling] Relax Transformer to allow rewriting macro expansions
Summary:
Currently, Transformer rejects any changes to source locations inside macro
expansions. This change relaxes that constraint to allow rewrites when the
entirety of the expansion is replaced, since that can be mapped to replacing the
entirety of the expansion range in the file source. This change makes
Transformer consistent with the handling of edit ranges in `clang::edit::Commit`
(which is used, for example, for applying `FixItHint`s from diagnostics).
Reviewers: ilya-biryukov
Subscribers: gribozavr, cfe-commits
Tags: #clang
Differential Revision: https://reviews.llvm.org/D64518
Modified:
cfe/trunk/lib/Tooling/Refactoring/Transformer.cpp
cfe/trunk/unittests/Tooling/TransformerTest.cpp
Modified: cfe/trunk/lib/Tooling/Refactoring/Transformer.cpp
URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Tooling/Refactoring/Transformer.cpp?rev=366473&r1=366472&r2=366473&view=diff
==============================================================================
--- cfe/trunk/lib/Tooling/Refactoring/Transformer.cpp (original)
+++ cfe/trunk/lib/Tooling/Refactoring/Transformer.cpp Thu Jul 18 10:44:54 2019
@@ -36,37 +36,6 @@ using llvm::StringError;
using MatchResult = MatchFinder::MatchResult;
-// Did the text at this location originate in a macro definition (aka. body)?
-// For example,
-//
-// #define NESTED(x) x
-// #define MACRO(y) { int y = NESTED(3); }
-// if (true) MACRO(foo)
-//
-// The if statement expands to
-//
-// if (true) { int foo = 3; }
-// ^ ^
-// Loc1 Loc2
-//
-// For SourceManager SM, SM.isMacroArgExpansion(Loc1) and
-// SM.isMacroArgExpansion(Loc2) are both true, but isOriginMacroBody(sm, Loc1)
-// is false, because "foo" originated in the source file (as an argument to a
-// macro), whereas isOriginMacroBody(SM, Loc2) is true, because "3" originated
-// in the definition of MACRO.
-static bool isOriginMacroBody(const clang::SourceManager &SM,
- clang::SourceLocation Loc) {
- while (Loc.isMacroID()) {
- if (SM.isMacroBodyExpansion(Loc))
- return true;
- // Otherwise, it must be in an argument, so we continue searching up the
- // invocation stack. getImmediateMacroCallerLoc() gives the location of the
- // argument text, inside the call text.
- Loc = SM.getImmediateMacroCallerLoc(Loc);
- }
- return false;
-}
-
Expected<SmallVector<tooling::detail::Transformation, 1>>
tooling::detail::translateEdits(const MatchResult &Result,
llvm::ArrayRef<ASTEdit> Edits) {
@@ -75,14 +44,17 @@ tooling::detail::translateEdits(const Ma
Expected<CharSourceRange> Range = Edit.TargetRange(Result);
if (!Range)
return Range.takeError();
- if (Range->isInvalid() ||
- isOriginMacroBody(*Result.SourceManager, Range->getBegin()))
+ llvm::Optional<CharSourceRange> EditRange =
+ getRangeForEdit(*Range, *Result.Context);
+ // FIXME: let user specify whether to treat this case as an error or ignore
+ // it as is currently done.
+ if (!EditRange)
return SmallVector<Transformation, 0>();
auto Replacement = Edit.Replacement(Result);
if (!Replacement)
return Replacement.takeError();
tooling::detail::Transformation T;
- T.Range = *Range;
+ T.Range = *EditRange;
T.Replacement = std::move(*Replacement);
Transformations.push_back(std::move(T));
}
Modified: cfe/trunk/unittests/Tooling/TransformerTest.cpp
URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/unittests/Tooling/TransformerTest.cpp?rev=366473&r1=366472&r2=366473&view=diff
==============================================================================
--- cfe/trunk/unittests/Tooling/TransformerTest.cpp (original)
+++ cfe/trunk/unittests/Tooling/TransformerTest.cpp Thu Jul 18 10:44:54 2019
@@ -137,7 +137,7 @@ protected:
TransformerTest() { appendToHeader(KHeaderContents); }
};
-// Given string s, change strlen($s.c_str()) to $s.size().
+// Given string s, change strlen($s.c_str()) to REPLACED.
static RewriteRule ruleStrlenSize() {
StringRef StringExpr = "strexpr";
auto StringType = namedDecl(hasAnyName("::basic_string", "::string"));
@@ -163,17 +163,6 @@ TEST_F(TransformerTest, NoMatch) {
testRule(ruleStrlenSize(), Input, Input);
}
-// Tests that expressions in macro arguments are rewritten (when applicable).
-TEST_F(TransformerTest, StrlenSizeMacro) {
- std::string Input = R"cc(
-#define ID(e) e
- int f(string s) { return ID(strlen(s.c_str())); })cc";
- std::string Expected = R"cc(
-#define ID(e) e
- int f(string s) { return ID(REPLACED); })cc";
- testRule(ruleStrlenSize(), Input, Expected);
-}
-
// Tests replacing an expression.
TEST_F(TransformerTest, Flag) {
StringRef Flag = "flag";
@@ -619,23 +608,114 @@ TEST_F(TransformerTest, ErrorOccurredMat
EXPECT_EQ(ErrorCount, 0);
}
-TEST_F(TransformerTest, NoTransformationInMacro) {
+// Transformation of macro source text when the change encompasses the entirety
+// of the expanded text.
+TEST_F(TransformerTest, SimpleMacro) {
+ std::string Input = R"cc(
+#define ZERO 0
+ int f(string s) { return ZERO; }
+ )cc";
+ std::string Expected = R"cc(
+#define ZERO 0
+ int f(string s) { return 999; }
+ )cc";
+
+ StringRef zero = "zero";
+ RewriteRule R = makeRule(integerLiteral(equals(0)).bind(zero),
+ change(node(zero), text("999")));
+ testRule(R, Input, Expected);
+}
+
+// Transformation of macro source text when the change encompasses the entirety
+// of the expanded text, for the case of function-style macros.
+TEST_F(TransformerTest, FunctionMacro) {
std::string Input = R"cc(
#define MACRO(str) strlen((str).c_str())
- int f(string s) { return MACRO(s); })cc";
- testRule(ruleStrlenSize(), Input, Input);
+ int f(string s) { return MACRO(s); }
+ )cc";
+ std::string Expected = R"cc(
+#define MACRO(str) strlen((str).c_str())
+ int f(string s) { return REPLACED; }
+ )cc";
+
+ testRule(ruleStrlenSize(), Input, Expected);
}
-// This test handles the corner case where a macro called within another macro
-// expands to matching code, but the matched code is an argument to the nested
-// macro. A simple check of isMacroArgExpansion() vs. isMacroBodyExpansion()
-// will get this wrong, and transform the code. This test verifies that no such
-// transformation occurs.
-TEST_F(TransformerTest, NoTransformationInNestedMacro) {
+// Tests that expressions in macro arguments can be rewritten.
+TEST_F(TransformerTest, MacroArg) {
+ std::string Input = R"cc(
+#define PLUS(e) e + 1
+ int f(string s) { return PLUS(strlen(s.c_str())); }
+ )cc";
+ std::string Expected = R"cc(
+#define PLUS(e) e + 1
+ int f(string s) { return PLUS(REPLACED); }
+ )cc";
+
+ testRule(ruleStrlenSize(), Input, Expected);
+}
+
+// Tests that expressions in macro arguments can be rewritten, even when the
+// macro call occurs inside another macro's definition.
+TEST_F(TransformerTest, MacroArgInMacroDef) {
std::string Input = R"cc(
#define NESTED(e) e
#define MACRO(str) NESTED(strlen((str).c_str()))
- int f(string s) { return MACRO(s); })cc";
+ int f(string s) { return MACRO(s); }
+ )cc";
+ std::string Expected = R"cc(
+#define NESTED(e) e
+#define MACRO(str) NESTED(strlen((str).c_str()))
+ int f(string s) { return REPLACED; }
+ )cc";
+
+ testRule(ruleStrlenSize(), Input, Expected);
+}
+
+// Tests the corner case of the identity macro, specifically that it is
+// discarded in the rewrite rather than preserved (like PLUS is preserved in the
+// previous test). This behavior is of dubious value (and marked with a FIXME
+// in the code), but we test it to verify (and demonstrate) how this case is
+// handled.
+TEST_F(TransformerTest, IdentityMacro) {
+ std::string Input = R"cc(
+#define ID(e) e
+ int f(string s) { return ID(strlen(s.c_str())); }
+ )cc";
+ std::string Expected = R"cc(
+#define ID(e) e
+ int f(string s) { return REPLACED; }
+ )cc";
+
+ testRule(ruleStrlenSize(), 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
+// suffice.
+TEST_F(TransformerTest, NoPartialRewriteOMacroExpansion) {
+ std::string Input = R"cc(
+#define ZERO_PLUS 0 + 3
+ int f(string s) { return ZERO_PLUS; })cc";
+
+ StringRef zero = "zero";
+ RewriteRule R = makeRule(integerLiteral(equals(0)).bind(zero),
+ change(node(zero), text("0")));
+ testRule(R, Input, Input);
+}
+
+// This test handles the corner case where a macro expands within another macro
+// to matching code, but that code is an argument to the nested macro call. A
+// simple check of isMacroArgExpansion() vs. isMacroBodyExpansion() will get
+// this wrong, and transform the code.
+TEST_F(TransformerTest, NoPartialRewriteOfMacroExpansionForMacroArgs) {
+ std::string Input = R"cc(
+#define NESTED(e) e
+#define MACRO(str) 1 + NESTED(strlen((str).c_str()))
+ int f(string s) { return MACRO(s); }
+ )cc";
+
testRule(ruleStrlenSize(), Input, Input);
}
} // namespace
More information about the cfe-commits
mailing list