hokein updated this revision to Diff 230905.
hokein marked 2 inline comments as done.
hokein added a comment.
address comments
- add related test for ast-based xrefs
- add comments to clarify using different location for symbols/references.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D70480/new/
https://reviews.llvm.org/D70480
Files:
clang-tools-extra/clangd/index/SymbolCollector.cpp
clang-tools-extra/clangd/unittests/SymbolCollectorTests.cpp
clang-tools-extra/clangd/unittests/XRefsTests.cpp
Index: clang-tools-extra/clangd/unittests/XRefsTests.cpp
===================================================================
--- clang-tools-extra/clangd/unittests/XRefsTests.cpp
+++ clang-tools-extra/clangd/unittests/XRefsTests.cpp
@@ -796,6 +796,19 @@
} // namespace ns
int main() { [[^ns]]::Foo foo; }
)cpp",
+
+ R"cpp(// Macros
+ #define TYPE(X) X
+ #define FOO Foo
+ #define CAT(X, Y) X##Y
+ class [[Fo^o]] {};
+ void test() {
+ TYPE([[Foo]]) foo;
+ [[FOO]] foo2;
+ TYPE(TYPE([[Foo]])) foo3;
+ [[CAT]](Fo, o) foo4;
+ }
+ )cpp",
};
for (const char *Test : Tests) {
Annotations T(Test);
Index: clang-tools-extra/clangd/unittests/SymbolCollectorTests.cpp
===================================================================
--- clang-tools-extra/clangd/unittests/SymbolCollectorTests.cpp
+++ clang-tools-extra/clangd/unittests/SymbolCollectorTests.cpp
@@ -644,6 +644,29 @@
HaveRanges(Header.ranges()))));
}
+TEST_F(SymbolCollectorTest, RefsOnMacros) {
+ // Refs collected from SymbolCollector behave in the same way as
+ // AST-based xrefs.
+ CollectorOpts.RefFilter = RefKind::All;
+ CollectorOpts.RefsInHeaders = true;
+ Annotations Header(R"(
+ #define TYPE(X) X
+ #define FOO Foo
+ #define CAT(X, Y) X##Y
+ class [[Foo]] {};
+ void test() {
+ TYPE([[Foo]]) foo;
+ [[FOO]] foo2;
+ TYPE(TYPE([[Foo]])) foo3;
+ [[CAT]](Fo, o) foo4;
+ }
+ )");
+ CollectorOpts.RefFilter = RefKind::All;
+ runSymbolCollector(Header.code(), "");
+ EXPECT_THAT(Refs, Contains(Pair(findSymbol(Symbols, "Foo").ID,
+ HaveRanges(Header.ranges()))));
+}
+
TEST_F(SymbolCollectorTest, HeaderAsMainFile) {
CollectorOpts.RefFilter = RefKind::All;
Annotations Header(R"(
Index: clang-tools-extra/clangd/index/SymbolCollector.cpp
===================================================================
--- clang-tools-extra/clangd/index/SymbolCollector.cpp
+++ clang-tools-extra/clangd/index/SymbolCollector.cpp
@@ -275,10 +275,9 @@
// Mark D as referenced if this is a reference coming from the main file.
// D may not be an interesting symbol, but it's cheaper to check at the end.
auto &SM = ASTCtx->getSourceManager();
- auto SpellingLoc = SM.getSpellingLoc(Loc);
if (Opts.CountReferences &&
(Roles & static_cast<unsigned>(index::SymbolRole::Reference)) &&
- SM.getFileID(SpellingLoc) == SM.getMainFileID())
+ SM.getFileID(SM.getSpellingLoc(Loc)) == SM.getMainFileID())
ReferencedDecls.insert(ND);
auto ID = getSymbolID(ND);
@@ -311,9 +310,13 @@
!shouldCollectSymbol(*ND, *ASTCtx, Opts, IsMainFileOnly))
return true;
// Do not store references to main-file symbols.
+ // Unlike other fields, e.g. Symbols (which use spelling locations), we use
+ // expansion locations for references comming from macros (as it aligns the
+ // behavior of clangd's AST-based xref).
if (CollectRef && !IsMainFileOnly && !isa<NamespaceDecl>(ND) &&
- (Opts.RefsInHeaders || SM.getFileID(SpellingLoc) == SM.getMainFileID()))
- DeclRefs[ND].emplace_back(SpellingLoc, Roles);
+ (Opts.RefsInHeaders ||
+ SM.getFileID(SM.getFileLoc(Loc)) == SM.getMainFileID()))
+ DeclRefs[ND].emplace_back(SM.getFileLoc(Loc), Roles);
// Don't continue indexing if this is a mere reference.
if (IsOnlyRef)
return true;
Index: clang-tools-extra/clangd/unittests/XRefsTests.cpp
===================================================================
--- clang-tools-extra/clangd/unittests/XRefsTests.cpp
+++ clang-tools-extra/clangd/unittests/XRefsTests.cpp
@@ -796,6 +796,19 @@
} // namespace ns
int main() { [[^ns]]::Foo foo; }
)cpp",
+
+ R"cpp(// Macros
+ #define TYPE(X) X
+ #define FOO Foo
+ #define CAT(X, Y) X##Y
+ class [[Fo^o]] {};
+ void test() {
+ TYPE([[Foo]]) foo;
+ [[FOO]] foo2;
+ TYPE(TYPE([[Foo]])) foo3;
+ [[CAT]](Fo, o) foo4;
+ }
+ )cpp",
};
for (const char *Test : Tests) {
Annotations T(Test);
Index: clang-tools-extra/clangd/unittests/SymbolCollectorTests.cpp
===================================================================
--- clang-tools-extra/clangd/unittests/SymbolCollectorTests.cpp
+++ clang-tools-extra/clangd/unittests/SymbolCollectorTests.cpp
@@ -644,6 +644,29 @@
HaveRanges(Header.ranges()))));
}
+TEST_F(SymbolCollectorTest, RefsOnMacros) {
+ // Refs collected from SymbolCollector behave in the same way as
+ // AST-based xrefs.
+ CollectorOpts.RefFilter = RefKind::All;
+ CollectorOpts.RefsInHeaders = true;
+ Annotations Header(R"(
+ #define TYPE(X) X
+ #define FOO Foo
+ #define CAT(X, Y) X##Y
+ class [[Foo]] {};
+ void test() {
+ TYPE([[Foo]]) foo;
+ [[FOO]] foo2;
+ TYPE(TYPE([[Foo]])) foo3;
+ [[CAT]](Fo, o) foo4;
+ }
+ )");
+ CollectorOpts.RefFilter = RefKind::All;
+ runSymbolCollector(Header.code(), "");
+ EXPECT_THAT(Refs, Contains(Pair(findSymbol(Symbols, "Foo").ID,
+ HaveRanges(Header.ranges()))));
+}
+
TEST_F(SymbolCollectorTest, HeaderAsMainFile) {
CollectorOpts.RefFilter = RefKind::All;
Annotations Header(R"(
Index: clang-tools-extra/clangd/index/SymbolCollector.cpp
===================================================================
--- clang-tools-extra/clangd/index/SymbolCollector.cpp
+++ clang-tools-extra/clangd/index/SymbolCollector.cpp
@@ -275,10 +275,9 @@
// Mark D as referenced if this is a reference coming from the main file.
// D may not be an interesting symbol, but it's cheaper to check at the end.
auto &SM = ASTCtx->getSourceManager();
- auto SpellingLoc = SM.getSpellingLoc(Loc);
if (Opts.CountReferences &&
(Roles & static_cast<unsigned>(index::SymbolRole::Reference)) &&
- SM.getFileID(SpellingLoc) == SM.getMainFileID())
+ SM.getFileID(SM.getSpellingLoc(Loc)) == SM.getMainFileID())
ReferencedDecls.insert(ND);
auto ID = getSymbolID(ND);
@@ -311,9 +310,13 @@
!shouldCollectSymbol(*ND, *ASTCtx, Opts, IsMainFileOnly))
return true;
// Do not store references to main-file symbols.
+ // Unlike other fields, e.g. Symbols (which use spelling locations), we use
+ // expansion locations for references comming from macros (as it aligns the
+ // behavior of clangd's AST-based xref).
if (CollectRef && !IsMainFileOnly && !isa<NamespaceDecl>(ND) &&
- (Opts.RefsInHeaders || SM.getFileID(SpellingLoc) == SM.getMainFileID()))
- DeclRefs[ND].emplace_back(SpellingLoc, Roles);
+ (Opts.RefsInHeaders ||
+ SM.getFileID(SM.getFileLoc(Loc)) == SM.getMainFileID()))
+ DeclRefs[ND].emplace_back(SM.getFileLoc(Loc), Roles);
// Don't continue indexing if this is a mere reference.
if (IsOnlyRef)
return true;
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
