[PATCH] D59481: [clangd] Count number of references while merging RefSlabs inside FileIndex

Kadir Cetinkaya via Phabricator via cfe-commits cfe-commits at lists.llvm.org
Tue Mar 19 03:23:59 PDT 2019


kadircet marked an inline comment as done.
kadircet added inline comments.


================
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;
----------------
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?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D59481/new/

https://reviews.llvm.org/D59481





More information about the cfe-commits mailing list