[PATCH] D95451: [clangd] references: decls of overrides of x are refs to x, not decls
This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
sammccall marked an inline comment as done.
Closed by commit rG8712df7a621d: [clangd] references: decls of overrides of x
are refs to x, not decls (authored by sammccall).
Changed prior to commit:
https://reviews.llvm.org/D95451?vs=319318=320479#toc
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D95451/new/
https://reviews.llvm.org/D95451
Files:
clang-tools-extra/clangd/XRefs.cpp
clang-tools-extra/clangd/index/Index.h
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
@@ -1871,7 +1871,7 @@
};
class Derived : public Base {
public:
- void $decl[[func]]() override; // FIXME: ref, not decl
+ void [[func]]() override;
};
void test(Derived* D) {
D->[[func]]();
Index: clang-tools-extra/clangd/index/Index.h
===
--- clang-tools-extra/clangd/index/Index.h
+++ clang-tools-extra/clangd/index/Index.h
@@ -104,11 +104,12 @@
lookup(const LookupRequest ,
llvm::function_ref Callback) const = 0;
- /// Finds all occurrences (e.g. references, declarations, definitions) of a
- /// symbol and applies \p Callback on each result.
+ /// Finds all occurrences (e.g. references, declarations, definitions) of
+ /// symbols and applies \p Callback on each result.
///
/// Results should be returned in arbitrary order.
/// The returned result must be deep-copied if it's used outside Callback.
+ /// FIXME: there's no indication which result references which symbol.
///
/// Returns true if there will be more results (limited by Req.Limit);
virtual bool refs(const RefsRequest ,
Index: clang-tools-extra/clangd/XRefs.cpp
===
--- clang-tools-extra/clangd/XRefs.cpp
+++ clang-tools-extra/clangd/XRefs.cpp
@@ -872,6 +872,7 @@
struct Reference {
syntax::Token SpelledTok;
index::SymbolRoleSet Role;
+SymbolID Target;
Range range(const SourceManager ) const {
return halfOpenToRange(SM, SpelledTok.range(SM).toCharRange(SM));
@@ -906,13 +907,15 @@
SourceLocation Loc,
index::IndexDataConsumer::ASTNodeInfo ASTNode) override {
const SourceManager = AST.getSourceManager();
-if (!isInsideMainFile(Loc, SM) ||
-TargetIDs.find(getSymbolID(D)) == TargetIDs.end())
+if (!isInsideMainFile(Loc, SM))
+ return true;
+SymbolID ID = getSymbolID(D);
+if (!TargetIDs.contains(ID))
return true;
const auto = AST.getTokens();
Loc = SM.getFileLoc(Loc);
if (const auto *Tok = TB.spelledTokenAt(Loc))
- References.push_back({*Tok, Roles});
+ References.push_back({*Tok, Roles, ID});
return true;
}
@@ -1297,7 +1300,7 @@
return {};
}
- RefsRequest Req;
+ llvm::DenseSet IDs, Overrides;
const auto *IdentifierAtCursor =
syntax::spelledIdentifierTouching(*CurLoc, AST.getTokens());
@@ -1322,7 +1325,7 @@
Results.References.push_back(std::move(Result));
}
}
- Req.IDs.insert(MacroSID);
+ IDs.insert(MacroSID);
}
} else {
// Handle references to Decls.
@@ -1336,7 +1339,6 @@
if (auto ID = getSymbolID(D))
Targets.insert(ID);
-llvm::DenseSet Overrides;
if (Index) {
RelationsRequest FindOverrides;
FindOverrides.Predicate = RelationKind::OverriddenBy;
@@ -1374,16 +1376,18 @@
ReferencesResult::Reference Result;
Result.Loc.range = Ref.range(SM);
Result.Loc.uri = URIMainFile;
- if (Ref.Role & static_cast(index::SymbolRole::Declaration))
-Result.Attributes |= ReferencesResult::Declaration;
- // clang-index doesn't report definitions as declarations, but they are.
- if (Ref.Role & static_cast(index::SymbolRole::Definition))
-Result.Attributes |=
-ReferencesResult::Definition | ReferencesResult::Declaration;
+ // Overrides are always considered references, not defs/decls.
+ if (!Overrides.contains(Ref.Target)) {
+if (Ref.Role & static_cast(index::SymbolRole::Declaration))
+ Result.Attributes |= ReferencesResult::Declaration;
+// clang-index doesn't report definitions as declarations, but they are.
+if (Ref.Role & static_cast(index::SymbolRole::Definition))
+ Result.Attributes |=
+ ReferencesResult::Definition | ReferencesResult::Declaration;
+ }
Results.References.push_back(std::move(Result));
}
if (Index && Results.References.size() <=
[PATCH] D95451: [clangd] references: decls of overrides of x are refs to x, not decls
kadircet accepted this revision.
kadircet added a comment.
This revision is now accepted and ready to land.
thanks, lgtm!
Comment at: clang-tools-extra/clangd/XRefs.cpp:1406
+ Req.Limit = Limit;
+ auto QueryIndex = [&](bool AllowAttributes) {
+if (Req.IDs.empty() || !Index || Results.References.size() > Limit)
it might be nicer to make `IDs` a parameter too.
Comment at: clang-tools-extra/clangd/XRefs.cpp:1409
+ return;
Results.HasMore |= Index->refs(Req, [&](const Ref ) {
// No need to continue process if we reach the limit.
just thinking out loud, i wonder why we don't just provide a symbolid in this
callback too. the interface currently says refs will be returned in any order,
and i am not sure about all the index implementations we have, but i think they
should be able to provide that for each reference. that way we could get rid of
multiple index queries (not that it matters too much currently, but it might
one day..)
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D95451/new/
https://reviews.llvm.org/D95451
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D95451: [clangd] references: decls of overrides of x are refs to x, not decls
sammccall created this revision.
sammccall added a reviewer: kadircet.
Herald added subscribers: usaxena95, arphaman.
sammccall requested review of this revision.
Herald added subscribers: cfe-commits, MaskRay, ilya-biryukov.
Herald added a project: clang.
This requires a second index query for refs to overrides, as the refs
call doesn't tell you which ref points at which symbol.
Repository:
rG LLVM Github Monorepo
https://reviews.llvm.org/D95451
Files:
clang-tools-extra/clangd/XRefs.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
@@ -1872,7 +1872,7 @@
};
class Derived : public Base {
public:
- void $decl[[func]]() override; // FIXME: ref, not decl
+ void [[func]]() override;
};
void test(Derived* D) {
D->[[func]]();
Index: clang-tools-extra/clangd/XRefs.cpp
===
--- clang-tools-extra/clangd/XRefs.cpp
+++ clang-tools-extra/clangd/XRefs.cpp
@@ -872,6 +872,7 @@
struct Reference {
syntax::Token SpelledTok;
index::SymbolRoleSet Role;
+SymbolID Target;
Range range(const SourceManager ) const {
return halfOpenToRange(SM, SpelledTok.range(SM).toCharRange(SM));
@@ -906,13 +907,15 @@
SourceLocation Loc,
index::IndexDataConsumer::ASTNodeInfo ASTNode) override {
const SourceManager = AST.getSourceManager();
-if (!isInsideMainFile(Loc, SM) ||
-TargetIDs.find(getSymbolID(D)) == TargetIDs.end())
+if (!isInsideMainFile(Loc, SM))
+ return true;
+SymbolID ID = getSymbolID(D);
+if (!TargetIDs.contains(ID))
return true;
const auto = AST.getTokens();
Loc = SM.getFileLoc(Loc);
if (const auto *Tok = TB.spelledTokenAt(Loc))
- References.push_back({*Tok, Roles});
+ References.push_back({*Tok, Roles, ID});
return true;
}
@@ -1298,6 +1301,7 @@
}
RefsRequest Req;
+ llvm::DenseSet Overrides;
const auto *IdentifierAtCursor =
syntax::spelledIdentifierTouching(*CurLoc, AST.getTokens());
@@ -1336,7 +1340,6 @@
if (auto ID = getSymbolID(D))
Targets.insert(ID);
-llvm::DenseSet Overrides;
if (Index) {
RelationsRequest FindOverrides;
FindOverrides.Predicate = RelationKind::OverriddenBy;
@@ -1374,16 +1377,18 @@
ReferencesResult::Reference Result;
Result.Loc.range = Ref.range(SM);
Result.Loc.uri = URIMainFile;
- if (Ref.Role & static_cast(index::SymbolRole::Declaration))
-Result.Attributes |= ReferencesResult::Declaration;
- // clang-index doesn't report definitions as declarations, but they are.
- if (Ref.Role & static_cast(index::SymbolRole::Definition))
-Result.Attributes |=
-ReferencesResult::Definition | ReferencesResult::Declaration;
+ // Overrides are always considered references, not defs/decls.
+ if (!Overrides.contains(Ref.Target)) {
+if (Ref.Role & static_cast(index::SymbolRole::Declaration))
+ Result.Attributes |= ReferencesResult::Declaration;
+// clang-index doesn't report definitions as declarations, but they are.
+if (Ref.Role & static_cast(index::SymbolRole::Definition))
+ Result.Attributes |=
+ ReferencesResult::Definition | ReferencesResult::Declaration;
+ }
Results.References.push_back(std::move(Result));
}
if (Index && Results.References.size() <= Limit) {
- Req.IDs = std::move(Overrides);
for (const Decl *D : Decls) {
// Not all symbols can be referenced from outside (e.g.
// function-locals).
@@ -1397,8 +1402,10 @@
}
}
// Now query the index for references from other files.
- if (!Req.IDs.empty() && Index && Results.References.size() <= Limit) {
-Req.Limit = Limit;
+ Req.Limit = Limit;
+ auto QueryIndex = [&](bool AllowAttributes) {
+if (Req.IDs.empty() || !Index || Results.References.size() > Limit)
+ return;
Results.HasMore |= Index->refs(Req, [&](const Ref ) {
// No need to continue process if we reach the limit.
if (Results.References.size() > Limit)
@@ -1409,15 +1416,22 @@
return;
ReferencesResult::Reference Result;
Result.Loc = std::move(*LSPLoc);
- if ((R.Kind & RefKind::Declaration) == RefKind::Declaration)
-Result.Attributes |= ReferencesResult::Declaration;
- // FIXME: our index should definitely store def | decl separately!
- if ((R.Kind & RefKind::Definition) == RefKind::Definition)
-Result.Attributes |=
-ReferencesResult::Declaration | ReferencesResult::Definition;
+ if (AllowAttributes) {
+if ((R.Kind &
