nridge added inline comments.
================
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;
----------------
kadircet wrote:
> We might need additional options, like limiting number of results. Could you
> instead accept a `RelsRequest` object? You can check `RefsRequest` for a
> sample
I called it `RelationsRequest` to avoid visual confusion with `RefsRequest`.
================
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)
----------------
kadircet wrote:
> why not just store `densemap<<s,p>,arrayref<o>>` ?
I used `std::vector<o>` because it wasn't clear where the array would live
otherwise.
================
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;
----------------
kadircet wrote:
> 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?
The description in `MergedIndex::refs()` says:
```
// We don't want duplicated refs from the static/dynamic indexes,
// and we can't reliably deduplicate them because offsets may differ slightly.
// We consider the dynamic index authoritative and report all its refs,
// and only report static index refs from other files.
```
It seems to me that the problem of "can't reliably deduplicate them because
offsets may differ slightly" doesn't apply to relations, as there are no
offsets involved. So, is there still a need to have additional logic here?
If so, what would the logic be -- don't return an object O1 from the static
index if we've returned an object O2 from the dynamic index defined in the same
file as O1? (Note that to implement this, we'd have to additionally look up
each SymbolID we return in the symbol table, as we don't otherwise know what
file the object is located in.)
================
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-
----------------
kadircet wrote:
> `+rels.size()`
`Size` is only the backing data size, so if we don't include the relations in
the backing data, we shouldn't include `Rels.size()`, right?
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D62839/new/
https://reviews.llvm.org/D62839
_______________________________________________
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
