ymandel created this revision.
ymandel added a reviewer: ilya-biryukov.
Herald added a project: clang.
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).
Repository:
rG LLVM Github Monorepo
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 0; }
+ )cc";
+
+ StringRef zero = "zero";
+ RewriteRule R = makeRule(integerLiteral(equals(0)).bind(zero),
+ change(node(zero), text("0")));
+ 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
