ioeric added inline comments.
================
Comment at: clang-tools-extra/clangd/index/FileIndex.cpp:33
+// If MergedSyms is provided, increments a symbols Reference count once for
+// every file it was mentioned in.
+void mergeRefs(llvm::ArrayRef<std::shared_ptr<RefSlab>> RefSlabs,
----------------
nit: s/every file/every slab/?
There is no "file" in this context.
================
Comment at: clang-tools-extra/clangd/index/FileIndex.cpp:46
+ auto It = MergedSyms->find(Sym.first);
+ assert(It != MergedSyms->end() && "Reference to unknown symbol!");
+ // Note that we increment References per each file mentioning the
----------------
I think we can trigger this assertion with an incomplete background index. For
example, we've seen references of a symbol in some files but we've not parsed
the file that contains the decls.
================
Comment at: clang-tools-extra/clangd/index/FileIndex.cpp:50
+ // FileIndex keeps only oneslab per file.
+ // This contradicts with behavior inside clangd-indexer, it only counts
+ // translation units since it processes each header multiple times.
----------------
kadircet wrote:
> ioeric wrote:
> > this is a bit of a code smell. FileIndex is not supposed to know anything
> > about clangd-indexer etc.
> I've actually just left the comment for review so that we can decide what to
> do about discrepancy(with my reasoning). It is not like FileIndex is somehow
> tied to clangd-indexer now?
the comment is useful. I just think it's in the wrong place. If we define the
reference counting behavior for `FileSymbols`, `FileSymbols`wouldn't need to
know about clangd-indexer or background-index. This comment can then live in
background index instead.
================
Comment at: clang-tools-extra/clangd/index/FileIndex.cpp:170
I.first->second = mergeSymbol(I.first->second, Sym);
+ // We re-count number of references while merging refs from scratch.
+ I.first->getSecond().References = 0;
----------------
kadircet wrote:
> ioeric wrote:
> > kadircet wrote:
> > > ioeric wrote:
> > > > note that references might not always exist. SymbolCollector can be set
> > > > up to not collect references.
> > > I thought we wouldn't make any promises about `References` in such a
> > > case, do we?
> > Is this documented somewhere?
> >
> > `References` and `RefSlab` have been two independent things. `References`
> > existed before `RefSlab`. Now might be a good time to unify them, but I
> > think we should think about how this is reflected in their definitions
> > (documentation etc) and other producers and consumers of the structures.
> Both of them are only produced by SymbolCollector, which has an option
> `CountReferences` with a comment saying `// Populate the Symbol.References
> field.`.
> Unfortunately that's not what it does, instead it always sets
> `Symol.References` field to `1` for every symbol collected during indexing of
> a single TU.
> Then it is up to the consumers to merge those `1`s. So even though we say:
> ```
> /// The number of translation units that reference this symbol from their main
> /// file. This number is only meaningful if aggregated in an index.```
> in the comments of `Symbol::References` it is totally up-to-the consumer who
> performs the merging.
>
> The option controls whether Symbol Occurences should be collected or not.
>
> I see two possible options:
> - Un-tie SymbolCollector from Symbol::References and rather define it as:
> ```
> /// This field is for the convenience of an aggregated symbol index
> ```
> - Rather change Index structure's(consumers of this information) to
> store(and build) this information internally if they plan to make use of it ?
>
> WDYT?
> Unfortunately that's not what it does, instead it always sets
> Symol.References field to 1 for every symbol collected during indexing of a
> single TU.
Looking at the code, I think `Symol.References` is indeed only populated (i.e.
set to 1) when `CountReferences` is enabled. Honestly, we should probably get
rid of `CountReferences`; I don't see a reason not to count references.
But the statement `count one per TU` still stands in SymbolCollector, as it
always only handles one TU at a time.
Actually, all I'm asking in the initial comment is to define the `References`
counting behavior when there is no reference ;) I think a reasonable solution
is to keep the `references` untouched when there is no reference, so that
`References` aggregation via `mergeSymbol` can still work (for one-TU-per-Slab
use cases).
And we should probably also document the new behavior e.g. `DuplicateHandling`
in FileIndex.h seems to be a good place.
================
Comment at: clang-tools-extra/clangd/index/FileIndex.cpp:180
}
// FIXME: aggregate symbol reference count based on references.
break;
----------------
this FIXME can be removed?
================
Comment at: clang-tools-extra/clangd/index/FileIndex.cpp:189
AllSymbols.push_back(&Sym);
+ mergeRefs(RefSlabs, RefsStorage, AllRefs);
break;
----------------
any reason not to count references for `PickOne`?
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D59481/new/
https://reviews.llvm.org/D59481
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
