diff --git a/clang/include/clang/Tooling/Transformer/SourceCode.h b/clang/include/clang/Tooling/Transformer/SourceCode.h --- a/clang/include/clang/Tooling/Transformer/SourceCode.h +++ b/clang/include/clang/Tooling/Transformer/SourceCode.h @@ -91,16 +91,25 @@ const SourceManager &SM); /// Attempts to resolve the given range to one that can be edited by a rewrite; -/// generally, one that starts and ends within a particular file. It supports a -/// limited set of cases involving source locations in macro expansions. If a -/// value is returned, it satisfies \c validateEditRange. +/// generally, one that starts and ends within a particular file. If a value is +/// returned, it satisfies \c validateEditRange. +/// +/// If \c IncludeMacroExpansion is true, a limited set of cases involving source +/// locations in macro expansions is supported. 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) llvm::Optional getRangeForEdit(const CharSourceRange &EditRange, const SourceManager &SM, - const LangOptions &LangOpts); + const LangOptions &LangOpts, bool IncludeMacroExpansion = true); inline llvm::Optional -getRangeForEdit(const CharSourceRange &EditRange, const ASTContext &Context) { +getRangeForEdit(const CharSourceRange &EditRange, const ASTContext &Context, + bool IncludeMacroExpansion = true) { return getRangeForEdit(EditRange, Context.getSourceManager(), - Context.getLangOpts()); + Context.getLangOpts(), IncludeMacroExpansion); } } // namespace tooling } // namespace clang diff --git a/clang/lib/Tooling/Transformer/SourceCode.cpp b/clang/lib/Tooling/Transformer/SourceCode.cpp --- a/clang/lib/Tooling/Transformer/SourceCode.cpp +++ b/clang/lib/Tooling/Transformer/SourceCode.cpp @@ -78,27 +78,46 @@ return llvm::Error::success(); } -llvm::Optional -clang::tooling::getRangeForEdit(const CharSourceRange &EditRange, - const SourceManager &SM, - const LangOptions &LangOpts) { - // 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. - CharSourceRange Range = Lexer::makeFileCharRange(EditRange, SM, LangOpts); +static bool SpelledInMacroDefinition(SourceLocation Loc, + const SourceManager &SM) { + while (Loc.isMacroID()) { + const auto &Expansion = SM.getSLocEntry(SM.getFileID(Loc)).getExpansion(); + if (Expansion.isMacroArgExpansion()) { + // Check the spelling location of the macro arg, in case the arg itself is + // in a macro expansion. + Loc = Expansion.getSpellingLoc(); + } else { + Loc = Expansion.getExpansionLocStart(); + if (Loc.isFileID()) + return true; + } + } + return false; +} + +llvm::Optional clang::tooling::getRangeForEdit( + const CharSourceRange &EditRange, const SourceManager &SM, + const LangOptions &LangOpts, bool IncludeMacroExpansion) { + CharSourceRange Range; + if (IncludeMacroExpansion) { + Range = Lexer::makeFileCharRange(EditRange, SM, LangOpts); + } else { + if (SpelledInMacroDefinition(EditRange.getBegin(), SM) || + SpelledInMacroDefinition(EditRange.getEnd(), SM)) + return std::nullopt; + + auto B = SM.getSpellingLoc(EditRange.getBegin()); + auto E = SM.getSpellingLoc(EditRange.getEnd()); + if (EditRange.isTokenRange()) { + E = Lexer::getLocForEndOfToken(E, 0, SM, LangOpts); + } + Range = CharSourceRange::getCharRange(B, E); + } + bool IsInvalid = llvm::errorToBool(validateEditRange(Range, SM)); if (IsInvalid) return std::nullopt; return Range; - } static bool startsWithNewline(const SourceManager &SM, const Token &Tok) { diff --git a/clang/unittests/Tooling/SourceCodeTest.cpp b/clang/unittests/Tooling/SourceCodeTest.cpp --- a/clang/unittests/Tooling/SourceCodeTest.cpp +++ b/clang/unittests/Tooling/SourceCodeTest.cpp @@ -453,7 +453,11 @@ Visitor.runOver(Code); } -TEST(SourceCodeTest, EditRangeWithMacroExpansionsShouldSucceed) { +class GetRangeForEditTest : public testing::TestWithParam {}; +INSTANTIATE_TEST_SUITE_P(WithAndWithoutExpansions, GetRangeForEditTest, + testing::Bool()); + +TEST_P(GetRangeForEditTest, EditRangeWithMacroExpansionsShouldSucceed) { // The call expression, whose range we are extracting, includes two macro // expansions. llvm::Annotations Code(R"cpp( @@ -463,10 +467,9 @@ )cpp"); CallsVisitor Visitor; - Visitor.OnCall = [&Code](CallExpr *CE, ASTContext *Context) { auto Range = CharSourceRange::getTokenRange(CE->getSourceRange()); - EXPECT_THAT(getRangeForEdit(Range, *Context), + EXPECT_THAT(getRangeForEdit(Range, *Context, GetParam()), ValueIs(AsRange(Context->getSourceManager(), Code.range("r")))); }; Visitor.runOver(Code.code()); @@ -487,7 +490,29 @@ Visitor.runOver(Code.code()); } -TEST(SourceCodeTest, EditPartialMacroExpansionShouldFail) { +TEST(SourceCodeTest, EditInvolvingExpansionIgnoringExpansionShouldFail) { + // If we specify to ignore macro expansions, none of these call expressions + // should have an editable range. + llvm::Annotations Code(R"cpp( +#define M1(x) x(1) +#define M2(x, y) x ## y +#define M3(x) foobar(x) +int foobar(int); +int a = M1(foobar); +int b = M2(foo, bar(2)); +int c = M3(3); +)cpp"); + + CallsVisitor Visitor; + Visitor.OnCall = [](CallExpr *CE, ASTContext *Context) { + auto Range = CharSourceRange::getTokenRange(CE->getSourceRange()); + EXPECT_FALSE( + getRangeForEdit(Range, *Context, /*IncludeMacroExpansion=*/false)); + }; + Visitor.runOver(Code.code()); +} + +TEST_P(GetRangeForEditTest, EditPartialMacroExpansionShouldFail) { std::string Code = R"cpp( #define BAR 10+ int c = BAR 3.0; @@ -496,12 +521,12 @@ IntLitVisitor Visitor; Visitor.OnIntLit = [](IntegerLiteral *Expr, ASTContext *Context) { auto Range = CharSourceRange::getTokenRange(Expr->getSourceRange()); - EXPECT_FALSE(getRangeForEdit(Range, *Context)); + EXPECT_FALSE(getRangeForEdit(Range, *Context, GetParam())); }; Visitor.runOver(Code); } -TEST(SourceCodeTest, EditWholeMacroArgShouldSucceed) { +TEST_P(GetRangeForEditTest, EditWholeMacroArgShouldSucceed) { llvm::Annotations Code(R"cpp( #define FOO(a) a + 7.0; int a = FOO($r[[10]]); @@ -510,13 +535,13 @@ IntLitVisitor Visitor; Visitor.OnIntLit = [&Code](IntegerLiteral *Expr, ASTContext *Context) { auto Range = CharSourceRange::getTokenRange(Expr->getSourceRange()); - EXPECT_THAT(getRangeForEdit(Range, *Context), + EXPECT_THAT(getRangeForEdit(Range, *Context, GetParam()), ValueIs(AsRange(Context->getSourceManager(), Code.range("r")))); }; Visitor.runOver(Code.code()); } -TEST(SourceCodeTest, EditPartialMacroArgShouldSucceed) { +TEST_P(GetRangeForEditTest, EditPartialMacroArgShouldSucceed) { llvm::Annotations Code(R"cpp( #define FOO(a) a + 7.0; int a = FOO($r[[10]] + 10.0); @@ -525,7 +550,7 @@ IntLitVisitor Visitor; Visitor.OnIntLit = [&Code](IntegerLiteral *Expr, ASTContext *Context) { auto Range = CharSourceRange::getTokenRange(Expr->getSourceRange()); - EXPECT_THAT(getRangeForEdit(Range, *Context), + EXPECT_THAT(getRangeForEdit(Range, *Context, GetParam()), ValueIs(AsRange(Context->getSourceManager(), Code.range("r")))); }; Visitor.runOver(Code.code()); @@ -541,7 +566,6 @@ )cpp"; CallsVisitor Visitor; - Visitor.OnCall = [](CallExpr *CE, ASTContext *Context) { auto Range = CharSourceRange::getTokenRange(CE->getSourceRange()); EXPECT_THAT_ERROR(validateEditRange(Range, Context->getSourceManager()),