[PATCH] D51279: [clangd] Implement findOccurrences interface in dynamic index.

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added inline comments.


================
Comment at: clangd/index/FileIndex.cpp:48
 
+  if (TopLevelDecls) { // index main AST, set occurrence flag.
+    CollectorOpts.OccurrenceFilter = SymbolOccurrenceKind::Declaration |
----------------
hokein wrote:
> ilya-biryukov wrote:
> > There is an implicit assumption (`TopLevelDecls != null` iff indexing the main file AST in dynamic index) that does not seem quite obvious.
> > Maybe add two overloads (for indexing the main AST and for indexing the preamble AST) or add an extra parameter to be explicit more about those choices?
> This is a hacky way to detect whether we are indexing main AST or preamble AST -- because the current `FileIndex::update(PathRef Path, ...)` interface doesn't contain this information, and we use it both for `PreambleAST` and `MainAST` indexing in the client side (`ParsingCallback`).
> 
> I think we might want two dedicated methods (`updateMainAST`, and `updatePreambleAST`) in `FileSymbol`, and I think we should address it in a follow-up patch. WDYT?
Either a dedicated methods or a separate parameter LG.
And it's probably ok to do it with a follow-up change if you want to keep the scope of this patch smaller. That should be a pretty small change overall, so it should be fine either way.


================
Comment at: clangd/index/FileIndex.h:100
 /// level decls obtained from \p AST are indexed.
-SymbolSlab
+std::pair<SymbolSlab, SymbolOccurrenceSlab>
 indexAST(ASTContext &AST, std::shared_ptr<Preprocessor> PP,
----------------
hokein wrote:
> ilya-biryukov wrote:
> > Maybe use a struct to allow named fields? Would mean a tiny amount of extra code in `indexAST`, but should make the client code slightly nicer.
> I think the type of `pair` explicitly express its meaning well enough? And this function is exposed for unittests, I'm happy to do the change if you insist.
Not really, that was just a suggestion. Feel free to ignore if the current version looks better to you.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D51279