[PATCH] D115243: [clangd] Extend SymbolOrigin, stop serializing it

Wed Dec 15 05:38:53 PST 2021

sammccall added a comment.

In D115243#3185393 <https://reviews.llvm.org/D115243#3185393>, @kadircet wrote:

> Yes and no. As mentioned SymbolSlabs are frozen once created and I totally agree with the reasons there. But AFAICT in none of the indexes (apart from monolithic FileIndex) we ever have a single SymbolSlab.
> We always have multiple of those and periodically build a single slab out of them, by copying/merging each symbol so that we can build a SymbolIndex on top of the final merged SymbolSlab. I was suggesting to perform ultimate Origin tweaking there (we can even respect the Origins of each individual slabs too).

Discussed this offline a bit.

We could pass around pairs of (SymbolSlab, Origin) and make it the index's responsibility to set the origin on each symbol. But I don't think this communicates so clearly that the origin reflects where the symbol came from/passed through, rather than what's serving it. (Given that Symbol::Origin exists in the slabs too).

> - Definitely less plumbing, we just let FileSymbol now what it should be called during construction. Background/RemoteIndex already has this knowledge hard coded.

Origin for an index isn't monolithic. Even a slab can contain symbols that are the result of merging, but FileIndex will soon have symbols from stdlib index + preamble index too.
Given this, I'm not sure it's much less plumbing - it's not just passing a constructor parameter to the index.

> - In theory those indexes that operate on top of a multiple SymbolSlabs should be inserting their own "origin" into the final merged slab as it contains some extra logic, just like MergeIndex.

This makes sense to me if it's useful, but I think this is *additional* to the origin reflecting the source of the slabs.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D115243