kadircet marked an inline comment as done.
kadircet added inline comments.
================
Comment at: clang-tools-extra/clangd/index/FileIndex.cpp:202
+ for (const auto &RelationSlab : RelationSlabs) {
+ for (const auto &R : *RelationSlab) {
+ AllRelations.push_back(R);
----------------
nit: braces
================
Comment at: clang-tools-extra/clangd/index/Index.h:76
+struct RelationsRequest {
+ SymbolID Subject;
----------------
let's have a limit as well
================
Comment at: clang-tools-extra/clangd/index/Index.h:139
+ void relations(const RelationsRequest &,
+ llvm::function_ref<void(const SymbolID &)>) const override;
+
----------------
thinking about this callback, I don't think there is any user that will make
use of the `SymbolID`s without querying for a `Symbol`.
So to reduce this boilerplate let's rather move this lookup into this API(all
`SymbolIndex` implementations provide `lookup` anyway so they all have a way to
convert a `SymbolID` into a `Symbol`) and change the callback to accept a
`const Symbol&` instead of a `SymbolID`
================
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;
----------------
nridge wrote:
> 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`.
yeah that's a good call thanks!
================
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;
----------------
nridge wrote:
> 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.)
yes I was referring to second part actually, but you are right we have no good
way of distinguishing stale information in this case. Let's leave it as it is,
but add a comment stating that we might return stale relations from static
index.
================
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-
----------------
nridge wrote:
> 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?
ok, but then let's make sure we include size of the densemap in
`estimateMemoryUsage` manually.
================
Comment at: clang-tools-extra/clangd/unittests/BackgroundIndexTests.cpp:220
+ // containing the definition of the subject (A_CC).
+ EXPECT_EQ(ShardHeader->Relations->size(), 1u);
----------------
could we instead check only relation is `A_CC isbaseof B_CC` ?
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
