[PATCH] D50385: [clangd] Collect symbol occurrences from AST.

[Haojian Wu via Phabricator via cfe-commits]

hokein added a comment.

In https://reviews.llvm.org/D50385#1191914, @ioeric wrote:

> 2 high-level questions:
>
> 1. What's the reason for having a separate `SymbolOccurrenceSlab`? Could store occurrences as extra payload of `Symbol`?


Storing occurrences in `Symbol` structure is easy to misuse by users IMO -- if we go through this way, we will end up having a `getOccurrences`-like method in `Symbol` structure. Once users get the `Symbol` instance, it is natural for them to call `getOccurrences` to get all occurrences of the symbol. However this `getOccurrences` method doesn't do what users expected (just returning an incomplete set of results or empty). To query the symbol occurrences, we should always use index interface.

Therefore, I think we should try to avoid these confusions in the design.

> 2. Could we merge `SymbolOccurrenceCollector` into the existing `SymbolCollector`? They look a lot alike. Having another index data consumer seems like more overhead on the user side.

The `SymbolOccurrenceCollector` has many responsibilities (collecting declaration, definition, code completion information etc), and the code is growing complex now. Merging the `SymbolOccurrenceCollector` to it will make it more complicated -- we will introduce more option flags like `collect-symbol-only`, `collect-occurrence-only` to configure it for our different use cases (we need to the implementation detail clearly in order to make a correct option for `SymbolCollector`). And I can foresee these two collectors might be run at different point (runWithPreamble vs runWithAST) in dynamic index.

They might use same facilities, but we could always share them.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D50385