Author: Eric Li Date: 2025-12-10T10:22:30-05:00 New Revision: feef80cab325165fafe60da1d31c23c8ccfc5a4a
URL: https://github.com/llvm/llvm-project/commit/feef80cab325165fafe60da1d31c23c8ccfc5a4a DIFF: https://github.com/llvm/llvm-project/commit/feef80cab325165fafe60da1d31c23c8ccfc5a4a.diff LOG: [clang][tooling] Fix `getFileRange` false negative (#171555) When an expression is in a single macro argument but also contains a macro, `getFileRange` would incorrectly reject that expression, concluding that it came from two different macro arguments because they came from two different expansions. We adjust the logic to look at the full path of macro argument expansion locations instead, tracking that if our traversal up the macro expansions continues all the way through macro arguments all the way to the top. This is similar to the technique used by `makeFileCharRange`. We also add some test cases to ensure we don't introduce any false positives. Added: Modified: clang/lib/Tooling/Transformer/SourceCode.cpp clang/unittests/Tooling/SourceCodeTest.cpp Removed: ################################################################################ diff --git a/clang/lib/Tooling/Transformer/SourceCode.cpp b/clang/lib/Tooling/Transformer/SourceCode.cpp index 7adceda05ad4f..0ca5113035f7b 100644 --- a/clang/lib/Tooling/Transformer/SourceCode.cpp +++ b/clang/lib/Tooling/Transformer/SourceCode.cpp @@ -86,32 +86,35 @@ llvm::Error clang::tooling::validateEditRange(const CharSourceRange &Range, return validateRange(Range, SM, /*AllowSystemHeaders=*/false); } -// Returns the location of the top-level macro argument that is the spelling for -// the expansion `Loc` is from. If `Loc` is spelled in the macro definition, -// returns an invalid `SourceLocation`. -static SourceLocation getMacroArgumentSpellingLoc(SourceLocation Loc, - const SourceManager &SM) { +// Returns the full set of expansion locations of `Loc` from bottom to top-most +// macro, if `Loc` is spelled in a macro argument. If `Loc` is spelled in the +// macro definition, returns an empty vector. +static llvm::SmallVector<SourceLocation, 2> +getMacroArgumentExpansionLocs(SourceLocation Loc, const SourceManager &SM) { assert(Loc.isMacroID() && "Location must be in a macro"); + llvm::SmallVector<SourceLocation, 2> ArgLocs; 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(); + ArgLocs.push_back(Expansion.getExpansionLocStart()); } else { return {}; } } - return Loc; + return ArgLocs; } static bool spelledInMacroDefinition(CharSourceRange Range, const SourceManager &SM) { if (Range.getBegin().isMacroID() && Range.getEnd().isMacroID()) { - // Check whether the range is entirely within a single macro argument. - auto B = getMacroArgumentSpellingLoc(Range.getBegin(), SM); - auto E = getMacroArgumentSpellingLoc(Range.getEnd(), SM); - return B.isInvalid() || B != E; + // Check whether the range is entirely within a single macro argument by + // checking if they are in the same macro argument at every level. + auto B = getMacroArgumentExpansionLocs(Range.getBegin(), SM); + auto E = getMacroArgumentExpansionLocs(Range.getEnd(), SM); + return B.empty() || B != E; } return Range.getBegin().isMacroID() || Range.getEnd().isMacroID(); diff --git a/clang/unittests/Tooling/SourceCodeTest.cpp b/clang/unittests/Tooling/SourceCodeTest.cpp index a998954a6e9ea..d0eba43b0bb1c 100644 --- a/clang/unittests/Tooling/SourceCodeTest.cpp +++ b/clang/unittests/Tooling/SourceCodeTest.cpp @@ -507,17 +507,21 @@ 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 NOOP(x) x #define M1(x) x(1) -#define M2(x, y) x ## y +#define M2(x, y) NOOP(M2_IMPL(x, y)) +#define M2_IMPL(x, y) x ## y #define M3(x) foobar(x) -#define M4(x, y) x y +#define M4(x, y) NOOP(x y) #define M5(x) x +#define M6(...) M4(__VA_ARGS__) int foobar(int); int a = M1(foobar); int b = M2(foo, bar(2)); int c = M3(3); int d = M4(foobar, (4)); int e = M5(foobar) (5); +int f = M6(foobar, (6)); )cpp"); CallsVisitor Visitor; @@ -592,18 +596,39 @@ int c = BAR 3.0; } TEST_P(GetFileRangeForEditTest, EditWholeMacroArgShouldSucceed) { - llvm::Annotations Code(R"cpp( -#define FOO(a) a + 7.0; -int a = FOO($r[[10]]); -)cpp"); + { + llvm::Annotations Code(R"cpp( + #define FOO(a) a + 7.0; + int a = FOO($r[[10]]); + )cpp"); + + IntLitVisitor Visitor; + Visitor.OnIntLit = [&Code](IntegerLiteral *Expr, ASTContext *Context) { + auto Range = CharSourceRange::getTokenRange(Expr->getSourceRange()); + EXPECT_THAT( + getFileRangeForEdit(Range, *Context, GetParam()), + ValueIs(AsRange(Context->getSourceManager(), Code.range("r")))); + }; + Visitor.runOver(Code.code()); + } - IntLitVisitor Visitor; - Visitor.OnIntLit = [&Code](IntegerLiteral *Expr, ASTContext *Context) { - auto Range = CharSourceRange::getTokenRange(Expr->getSourceRange()); - EXPECT_THAT(getFileRangeForEdit(Range, *Context, GetParam()), - ValueIs(AsRange(Context->getSourceManager(), Code.range("r")))); - }; - Visitor.runOver(Code.code()); + { + llvm::Annotations Code(R"cpp( + #define FOO(a) a + 7.0; + #define DO_NOTHING(x) x + int Frob(int x); + int a = FOO($r[[Frob(DO_NOTHING(40))]]); + )cpp"); + + CallsVisitor Visitor; + Visitor.OnCall = [&Code](CallExpr *Expr, ASTContext *Context) { + auto Range = CharSourceRange::getTokenRange(Expr->getSourceRange()); + EXPECT_THAT( + getFileRangeForEdit(Range, *Context, GetParam()), + ValueIs(AsRange(Context->getSourceManager(), Code.range("r")))); + }; + Visitor.runOver(Code.code()); + } } TEST_P(GetFileRangeForEditTest, EditPartialMacroArgShouldSucceed) { _______________________________________________ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
