ymandel updated this revision to Diff 210576.
ymandel added a comment.
remove unneeded include
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D64518/new/
https://reviews.llvm.org/D64518
Files:
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
@@ -137,7 +137,7 @@
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 @@
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 @@
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);
+}
+
+// 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);
}
-// 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, 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
Index: clang/lib/Tooling/Refactoring/Transformer.cpp
===================================================================
--- clang/lib/Tooling/Refactoring/Transformer.cpp
+++ clang/lib/Tooling/Refactoring/Transformer.cpp
@@ -36,37 +36,6 @@
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 @@
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));
}
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
