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

[Haojian Wu via Phabricator via cfe-commits]

hokein added a comment.

Thanks for the comments.



================
Comment at: clangd/index/FileIndex.cpp:48
 
+  if (TopLevelDecls) { // index main AST, set occurrence flag.
+    CollectorOpts.OccurrenceFilter = SymbolOccurrenceKind::Declaration |
----------------
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?


================
Comment at: clangd/index/FileIndex.h:50
 
+  std::vector<std::shared_ptr<SymbolOccurrenceSlab>> allOccurrenceSlabs() const;
+
----------------
sammccall wrote:
> sammccall wrote:
> > This return type seems inconsistent with the design of this class.
> > The purpose of the `shared_ptr<vector<symbol_slab>>` in `allSymbols` is to avoid exposing the concrete storage.
> > The analogue would I guess be `shared_ptr<DenseMap<SymbolID, vector<SymbolOccurrence*>>>`.
> > 
> > The cost of creating that does seem a little gross though.
> allOccurrences
> (name should reflect the semantics, not the type)
> The cost of creating that does seem a little gross though.

Previously I tried to avoid the cost of iterating/copying all occurrences. After some investigations, it seems that the number of symbol occurrences is small (~500 for `SemaDecl.cpp`, < 100 for `CodeComplete.cpp`). The cost should not be too expensive.


================
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,
----------------
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.


================
Comment at: clangd/index/MemIndex.cpp:36
   auto Idx = llvm::make_unique<MemIndex>();
-  Idx->build(getSymbolsFromSlab(std::move(Slab)));
+  Idx->build(getSymbolsFromSlab(std::move(Slab)), {});
   return std::move(Idx);
----------------
sammccall wrote:
> we should find a way to avoid having functions implicitly zero out parts of the index like this!
Made it explicit.


================
Comment at: clangd/index/Merge.cpp:95
+    Static->findOccurrences(Req, [&](const SymbolOccurrence& O) {
+      if (llvm::is_contained(SeenFiles, O.Location.FileURI))
+        return;
----------------
ilya-biryukov wrote:
> `llvm::is_contained` seems to be linear time, maybe replace with hash-table lookup.
> Or am I missing something?
Good catch, I thought it had a trait for `Map`, `Set`, but it turns out that `is_contained` is only a `std::find` wrapper :(


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D51279