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