kadircet created this revision.
kadircet added a reviewer: sammccall.
Herald added a subscriber: arphaman.
Herald added a project: All.
kadircet requested review of this revision.
Herald added projects: clang, clang-tools-extra.
Herald added a subscriber: cfe-commits.
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.
Repository:
rG LLVM Github Monorepo
https://reviews.llvm.org/D154335
Files:
clang-tools-extra/clangd/unittests/tweaks/AddUsingTests.cpp
clang/lib/Tooling/Syntax/Tokens.cpp
clang/unittests/Tooling/Syntax/TokensTest.cpp
Index: clang/unittests/Tooling/Syntax/TokensTest.cpp
===================================================================
--- clang/unittests/Tooling/Syntax/TokensTest.cpp
+++ clang/unittests/Tooling/Syntax/TokensTest.cpp
@@ -769,12 +769,15 @@
// 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 @@
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) {
Index: clang/lib/Tooling/Syntax/Tokens.cpp
===================================================================
--- clang/lib/Tooling/Syntax/Tokens.cpp
+++ clang/lib/Tooling/Syntax/Tokens.cpp
@@ -103,66 +103,13 @@
// 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 different 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 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()) {
Index: clang-tools-extra/clangd/unittests/tweaks/AddUsingTests.cpp
===================================================================
--- clang-tools-extra/clangd/unittests/tweaks/AddUsingTests.cpp
+++ clang-tools-extra/clangd/unittests/tweaks/AddUsingTests.cpp
@@ -71,6 +71,13 @@
// 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.
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
