[clang] 9841daf - [clang][tooling] Fix early termination when there are nested expansions
Kadir Cetinkaya via cfe-commits
cfe-commits at lists.llvm.org
Mon Jul 3 07:58:32 PDT 2023
Author: Kadir Cetinkaya
Date: 2023-07-03T16:58:04+02:00
New Revision: 9841daf27076886c6ab8e155eb812bda76f77532
URL: https://github.com/llvm/llvm-project/commit/9841daf27076886c6ab8e155eb812bda76f77532
DIFF: https://github.com/llvm/llvm-project/commit/9841daf27076886c6ab8e155eb812bda76f77532.diff
LOG: [clang][tooling] Fix early termination when there are nested expansions
This also does some cleanups, I am happy to undo them (or send as
separate patches):
- Change the early exit to stop only once we hit an expansion inside the
main file, to make sure we keep following the nested expansions.
- Add more tests to cover all the cases mentioned in the implementation
- Drop the adjustments for prev/next tokens. We do the final checks
based on the expansion locations anyway, so any intermediate mapping
was a no-op.
Differential Revision: https://reviews.llvm.org/D154335
Added:
Modified:
clang-tools-extra/clangd/unittests/tweaks/AddUsingTests.cpp
clang/lib/Tooling/Syntax/Tokens.cpp
clang/unittests/Tooling/Syntax/TokensTest.cpp
Removed:
################################################################################
diff --git a/clang-tools-extra/clangd/unittests/tweaks/AddUsingTests.cpp b/clang-tools-extra/clangd/unittests/tweaks/AddUsingTests.cpp
index da76ecad145548..1fd2487378d705 100644
--- a/clang-tools-extra/clangd/unittests/tweaks/AddUsingTests.cpp
+++ b/clang-tools-extra/clangd/unittests/tweaks/AddUsingTests.cpp
@@ -71,6 +71,13 @@ class cc {
// NestedNameSpecifier, but no namespace.
EXPECT_UNAVAILABLE(Header + "class Foo {}; class F^oo foo;");
+ // Nested macro case.
+ EXPECT_AVAILABLE(R"cpp(
+ #define ID2(X) X
+ #define ID(Y, X) Y;ID2(X)
+ namespace ns { struct Foo{}; }
+ ID(int xyz, ns::F^oo) f;)cpp");
+
// Check that we do not trigger in header files.
FileName = "test.h";
ExtraArgs.push_back("-xc++-header"); // .h file is treated a C by default.
diff --git a/clang/lib/Tooling/Syntax/Tokens.cpp b/clang/lib/Tooling/Syntax/Tokens.cpp
index 64e6eee6b62f2f..9c2f470e985fb6 100644
--- a/clang/lib/Tooling/Syntax/Tokens.cpp
+++ b/clang/lib/Tooling/Syntax/Tokens.cpp
@@ -103,66 +103,13 @@ SourceRange spelledForExpandedSlow(SourceLocation First, SourceLocation Last,
// The token `a` is wrapped in 4 arg-expansions, we only want to unwrap 2.
// We distinguish them by whether the macro expands into the target file.
// Fortunately, the target file ones will always appear first.
- auto &ExpMacro =
- SM.getSLocEntry(SM.getFileID(ExpFirst.getExpansionLocStart()))
- .getExpansion();
- if (ExpMacro.getExpansionLocStart().isMacroID())
+ auto ExpFileID = SM.getFileID(ExpFirst.getExpansionLocStart());
+ if (ExpFileID == TargetFile)
break;
// Replace each endpoint with its spelling inside the macro arg.
// (This is getImmediateSpellingLoc without repeating lookups).
First = ExpFirst.getSpellingLoc().getLocWithOffset(DecFirst.second);
Last = ExpLast.getSpellingLoc().getLocWithOffset(DecLast.second);
-
- // Now: how do we adjust the previous/next bounds? Three cases:
- // A) If they are also part of the same macro arg, we translate them too.
- // This will ensure that we don't select any macros nested within the
- // macro arg that cover extra tokens. Critical case:
- // #define ID(X) X
- // ID(prev target) // selecting 'target' succeeds
- // #define LARGE ID(prev target)
- // LARGE // selecting 'target' fails.
- // B) They are not in the macro at all, then their expansion range is a
- // sibling to it, and we can safely substitute that.
- // #define PREV prev
- // #define ID(X) X
- // PREV ID(target) // selecting 'target' succeeds.
- // #define LARGE PREV ID(target)
- // LARGE // selecting 'target' fails.
- // C) They are in a
diff erent arg of this macro, or the macro body.
- // Now selecting the whole macro arg is fine, but the whole macro is not.
- // Model this by setting using the edge of the macro call as the bound.
- // #define ID2(X, Y) X Y
- // ID2(prev, target) // selecting 'target' succeeds
- // #define LARGE ID2(prev, target)
- // LARGE // selecting 'target' fails
- auto AdjustBound = [&](SourceLocation &Bound) {
- if (Bound.isInvalid() || !Bound.isMacroID()) // Non-macro must be case B.
- return;
- auto DecBound = SM.getDecomposedLoc(Bound);
- auto &ExpBound = SM.getSLocEntry(DecBound.first).getExpansion();
- if (ExpBound.isMacroArgExpansion() &&
- ExpBound.getExpansionLocStart() == ExpFirst.getExpansionLocStart()) {
- // Case A: translate to (spelling) loc within the macro arg.
- Bound = ExpBound.getSpellingLoc().getLocWithOffset(DecBound.second);
- return;
- }
- while (Bound.isMacroID()) {
- SourceRange Exp = SM.getImmediateExpansionRange(Bound).getAsRange();
- if (Exp.getBegin() == ExpMacro.getExpansionLocStart()) {
- // Case B: bounds become the macro call itself.
- Bound = (&Bound == &Prev) ? Exp.getBegin() : Exp.getEnd();
- return;
- }
- // Either case C, or expansion location will later find case B.
- // We choose the upper bound for Prev and the lower one for Next:
- // ID(prev) target ID(next)
- // ^ ^
- // new-prev new-next
- Bound = (&Bound == &Prev) ? Exp.getEnd() : Exp.getBegin();
- }
- };
- AdjustBound(Prev);
- AdjustBound(Next);
}
// In all remaining cases we need the full containing macros.
@@ -170,9 +117,10 @@ SourceRange spelledForExpandedSlow(SourceLocation First, SourceLocation Last,
SourceRange Candidate =
SM.getExpansionRange(SourceRange(First, Last)).getAsRange();
auto DecFirst = SM.getDecomposedExpansionLoc(Candidate.getBegin());
- auto DecLast = SM.getDecomposedLoc(Candidate.getEnd());
+ auto DecLast = SM.getDecomposedExpansionLoc(Candidate.getEnd());
// Can end up in the wrong file due to bad input or token-pasting shenanigans.
- if (Candidate.isInvalid() || DecFirst.first != TargetFile || DecLast.first != TargetFile)
+ if (Candidate.isInvalid() || DecFirst.first != TargetFile ||
+ DecLast.first != TargetFile)
return SourceRange();
// Check bounds, which may still be inside macros.
if (Prev.isValid()) {
diff --git a/clang/unittests/Tooling/Syntax/TokensTest.cpp b/clang/unittests/Tooling/Syntax/TokensTest.cpp
index d0164677560067..0c08318a637c0b 100644
--- a/clang/unittests/Tooling/Syntax/TokensTest.cpp
+++ b/clang/unittests/Tooling/Syntax/TokensTest.cpp
@@ -769,12 +769,15 @@ TEST_F(TokenBufferTest, SpelledByExpanded) {
// Critical cases for mapping of Prev/Next in spelledForExpandedSlow.
recordTokens(R"cpp(
#define ID(X) X
- ID(prev ID(good))
+ ID(prev good)
+ ID(prev ID(good2))
#define LARGE ID(prev ID(bad))
LARGE
)cpp");
EXPECT_THAT(Buffer.spelledForExpanded(findExpanded("good")),
ValueIs(SameRange(findSpelled("good"))));
+ EXPECT_THAT(Buffer.spelledForExpanded(findExpanded("good2")),
+ ValueIs(SameRange(findSpelled("good2"))));
EXPECT_EQ(Buffer.spelledForExpanded(findExpanded("bad")), std::nullopt);
recordTokens(R"cpp(
@@ -785,19 +788,32 @@ TEST_F(TokenBufferTest, SpelledByExpanded) {
LARGE
)cpp");
EXPECT_THAT(Buffer.spelledForExpanded(findExpanded("good")),
- ValueIs(SameRange(findSpelled("good"))));
+ ValueIs(SameRange(findSpelled("good"))));
EXPECT_EQ(Buffer.spelledForExpanded(findExpanded("bad")), std::nullopt);
recordTokens(R"cpp(
#define ID(X) X
#define ID2(X, Y) X Y
- ID2(prev, ID(good))
+ ID2(prev, good)
+ ID2(prev, ID(good2))
#define LARGE ID2(prev, bad)
LARGE
)cpp");
EXPECT_THAT(Buffer.spelledForExpanded(findExpanded("good")),
- ValueIs(SameRange(findSpelled("good"))));
+ ValueIs(SameRange(findSpelled("good"))));
+ EXPECT_THAT(Buffer.spelledForExpanded(findExpanded("good2")),
+ ValueIs(SameRange(findSpelled("good2"))));
EXPECT_EQ(Buffer.spelledForExpanded(findExpanded("bad")), std::nullopt);
+
+ // Prev from macro body.
+ recordTokens(R"cpp(
+ #define ID(X) X
+ #define ID2(X, Y) X prev ID(Y)
+ ID2(not_prev, good)
+ )cpp");
+ EXPECT_THAT(Buffer.spelledForExpanded(findExpanded("good")),
+ ValueIs(SameRange(findSpelled("good"))));
+ EXPECT_EQ(Buffer.spelledForExpanded(findExpanded("prev good")), std::nullopt);
}
TEST_F(TokenBufferTest, ExpandedTokensForRange) {
More information about the cfe-commits
mailing list