ymandel updated this revision to Diff 210148.
ymandel added a comment.
tweaks in response to comments.
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
@@ -12,6 +12,7 @@
#include "clang/ASTMatchers/ASTMatchers.h"
#include "clang/Basic/Diagnostic.h"
#include "clang/Basic/SourceLocation.h"
+#include "clang/Lex/Lexer.h"
#include "clang/Rewrite/Core/Rewriter.h"
#include "clang/Tooling/Refactoring/AtomicChange.h"
#include "clang/Tooling/Refactoring/SourceCode.h"
@@ -67,6 +68,43 @@
return false;
}
+// Attempts to resolve the given range to one that can be edited by a rewrite.
+// That is, one that starts and ends within a particular file. This validation
+// allows for a limited set of projections from macro expansions back to the
+// expansion points.
+static llvm::Optional<CharSourceRange>
+makeValidRange(CharSourceRange Range, const MatchResult &Result) {
+ const SourceManager &SM = *Result.SourceManager;
+ // FIXME: makeFileCharRange() has the disadvantage of stripping off "identity"
+ // macros. For example, if we're looking to rewrite the int literal 3 to 6,
+ // and we have the following definition:
+ // #define DO_NOTHING(x) x
+ // then
+ // foo(DO_NOTHING(3))
+ // will be rewritten to
+ // foo(6)
+ // rather than the arguably better
+ // foo(DO_NOTHING(6))
+ // Decide whether the current behavior is desirable and modify if not.
+ Range = Lexer::makeFileCharRange(Range, SM, Result.Context->getLangOpts());
+ if (Range.isInvalid())
+ return None;
+
+ if (Range.getBegin().isMacroID() || Range.getEnd().isMacroID())
+ return None;
+ if (SM.isInSystemHeader(Range.getBegin()) ||
+ SM.isInSystemHeader(Range.getEnd()))
+ return None;
+
+ std::pair<FileID, unsigned> BeginInfo = SM.getDecomposedLoc(Range.getBegin());
+ std::pair<FileID, unsigned> EndInfo = SM.getDecomposedLoc(Range.getEnd());
+ if (BeginInfo.first != EndInfo.first ||
+ BeginInfo.second > EndInfo.second)
+ return None;
+
+ return Range;
+}
+
Expected<SmallVector<tooling::detail::Transformation, 1>>
tooling::detail::translateEdits(const MatchResult &Result,
llvm::ArrayRef<ASTEdit> Edits) {
@@ -75,14 +113,14 @@
Expected<CharSourceRange> Range = Edit.TargetRange(Result);
if (!Range)
return Range.takeError();
- if (Range->isInvalid() ||
- isOriginMacroBody(*Result.SourceManager, Range->getBegin()))
+ llvm::Optional<CharSourceRange> EditRange = makeValidRange(*Range, Result);
+ 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
