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

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added a comment.

The memory usage looks good. Some NITs and a major consideration wrt to the implementation of merging occurences from dynamic and static index.



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


================
Comment at: clangd/index/FileIndex.cpp:74
+  if (!Slab) {
     FileToSlabs.erase(Path);
+  } else {
----------------
NIT: maybe remove braces around single-statement branches?


================
Comment at: clangd/index/FileIndex.cpp:126
     auto Slab = llvm::make_unique<SymbolSlab>();
-    *Slab = indexAST(*AST, PP, TopLevelDecls, URISchemes);
-    FSymbols.update(Path, std::move(Slab));
+    auto IndexResults = indexAST(*AST, PP, TopLevelDecls, URISchemes);
+    *Slab = std::move(IndexResults.first);
----------------
Why not `std::tie(*Slab, *OccurenceSlab) = indexAST(...)`?
I assume this should work and give the optimal performance


================
Comment at: clangd/index/FileIndex.h:45
+  void update(PathRef Path, std::unique_ptr<SymbolSlab> Slab,
+              std::unique_ptr<SymbolOccurrenceSlab> Occurrences = nullptr);
 
----------------
Maybe avoid default arguments? Having clients pass nullptr explicitly seems like the right thing to do


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


================
Comment at: clangd/index/MemIndex.h:27
+  build(std::shared_ptr<std::vector<const Symbol *>> Symbols,
+        std::vector<std::shared_ptr<SymbolOccurrenceSlab>> OccurrenceSlabs = {});
 
----------------
Maybe remove the default parameter? Making the callers specify the occurrences explicitly could make their code more straight-forward to follow.


================
Comment at: clangd/index/Merge.cpp:88
+    // in seen files.
+    // FIXME: If a file has been updated, and there are no occurrences indexed
+    // in dynamic index, stale results are still returned (from static index).
----------------
Do we want to fix it before checking this in? As discussed offline, this seems to be a limitation that could affect the design (FileOccurrences vs SymbolOccurrence in the symbol interface). In case we want to avoid refactoring the interfaces later.


================
Comment at: clangd/index/Merge.cpp:95
+    Static->findOccurrences(Req, [&](const SymbolOccurrence& O) {
+      if (llvm::is_contained(SeenFiles, O.Location.FileURI))
+        return;
----------------
`llvm::is_contained` seems to be linear time, maybe replace with hash-table lookup.
Or am I missing something?


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D51279