kadircet added inline comments.
================
Comment at: clang-tools-extra/clangd/index/Background.cpp:293
+ // This map is used to figure out where to store relations.
+ llvm::DenseMap<SymbolID, File *> IDsToFiles;
for (const auto &Sym : *Index.Symbols) {
----------------
nit: rename to `SymbolIDToFile`?
================
Comment at: clang-tools-extra/clangd/index/Background.cpp:329
+ const auto FileIt = IDsToFiles.find(Rel.Subject);
+ if (FileIt != IDsToFiles.end()) {
+ FileIt->second->Relations.insert(&Rel);
----------------
nit: braces
================
Comment at: clang-tools-extra/clangd/index/FileIndex.cpp:201
+ std::vector<Relation> AllRelations;
+ {
+ for (const auto &RelationSlab : RelationSlabs) {
----------------
redundant scope
================
Comment at: clang-tools-extra/clangd/index/FileIndex.cpp:225
std::make_tuple(std::move(SymbolSlabs), std::move(RefSlabs),
- std::move(RefsStorage), std::move(SymsStorage)),
+ std::move(RelationSlabs), std::move(RefsStorage),
+ std::move(SymsStorage)),
----------------
no need to pass `RelationSlabs` in here `AllRelations` are just value types.
================
Comment at: clang-tools-extra/clangd/index/FileIndex.cpp:250
Path, llvm::make_unique<SymbolSlab>(std::move(Symbols)),
- llvm::make_unique<RefSlab>(), /*CountReferences=*/false);
+ llvm::make_unique<RefSlab>(), llvm::make_unique<RelationSlab>(),
+ /*CountReferences=*/false);
----------------
I think we need to pass relationslab in here. Since we might miss relations
like the following that are outside the main file:
```
class A {};
class B : public A {};
```
Would be glad if you could prove me right/wrong with a unittest as well.
================
Comment at: clang-tools-extra/clangd/index/Index.h:107
+ virtual void
+ relations(const SymbolID &Subject, index::SymbolRole Predicate,
+ llvm::function_ref<void(const SymbolID &)> Callback) const = 0;
----------------
We might need additional options, like limiting number of results. Could you
instead accept a `RelsRequest` object? You can check `RefsRequest` for a sample
================
Comment at: clang-tools-extra/clangd/index/MemIndex.cpp:22
// Store Slab size before it is moved.
const auto BackingDataSize = Slab.bytes() + Refs.bytes();
+ auto Data =
----------------
`+ Relations.bytes()`
================
Comment at: clang-tools-extra/clangd/index/MemIndex.cpp:24
+ auto Data =
+ std::make_tuple(std::move(Slab), std::move(Refs), std::move(Relations));
+ return llvm::make_unique<MemIndex>(std::get<0>(Data), std::get<1>(Data),
----------------
no need to put `Relations` into `Data` they are just values, right?
================
Comment at: clang-tools-extra/clangd/index/MemIndex.cpp:94
+ auto Rels = Relations.lookup(Subject, Predicate);
+ for (const auto &R : Rels) {
+ Callback(R.Object);
----------------
nit: braces
================
Comment at: clang-tools-extra/clangd/index/MemIndex.h:29
this->Refs.try_emplace(R.first, R.second.begin(), R.second.end());
+ RelationSlab::Builder Builder;
+ for (const Relation &R : Relations)
----------------
why not just store `densemap<<s,p>,arrayref<o>>` ?
================
Comment at: clang-tools-extra/clangd/index/Merge.cpp:126
+ llvm::function_ref<void(const SymbolID &)> Callback) const {
+ // Return results from both indexes but avoid duplicates.
+ llvm::DenseSet<SymbolID> SeenObjects;
----------------
avoiding deduplicates is not enough, we also wouldn't want to report stale
relations coming from static index.
Could you check the logic in `MergedIndex::refs`, and hopefully refactor it
into a class to share between these two?
================
Comment at: clang-tools-extra/clangd/index/dex/Dex.cpp:28
+ RelationSlab Rels) {
auto Size = Symbols.bytes() + Refs.bytes();
+ // There is no need to include "Rels" in Data because the relations are self-
----------------
`+rels.size()`
================
Comment at: clang-tools-extra/clangd/index/dex/Dex.cpp:269
+ auto Rels = Relations.lookup(Subject, Predicate);
+ for (const auto &R : Rels) {
+ Callback(R.Object);
----------------
nit: braces
================
Comment at: clang-tools-extra/clangd/index/dex/Dex.h:110
llvm::DenseMap<SymbolID, llvm::ArrayRef<Ref>> Refs;
+ RelationSlab Relations;
std::shared_ptr<void> KeepAlive; // poor man's move-only std::any
----------------
let's store `densemap<<s,p>, arrayref<o>>` here as well
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D62839/new/
https://reviews.llvm.org/D62839
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
