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

[Sam McCall via Phabricator via cfe-commits]

sammccall accepted this revision.
sammccall added a comment.
This revision is now accepted and ready to land.

This basically looks good to go (some fixes needed but they're pretty clear I think let me know if not!)



================
Comment at: clangd/index/FileIndex.cpp:63
+  auto Occurrences = Collector.takeOccurrences();
+  vlog("index for AST: \n"
+       "  symbol slab: {0} symbols, {1} bytes\n"
----------------
for this log to be useful, you might want to include the filename.
You can get it from the sourcemanager on the ASTContext.


================
Comment at: clangd/index/MemIndex.cpp:35
 
 std::unique_ptr<SymbolIndex> MemIndex::build(SymbolSlab Slab) {
   auto Idx = llvm::make_unique<MemIndex>();
----------------
This is still implicitly creating an index with no occurrences. Did you mean to accept a SymbolOccurrencesSlab here?


================
Comment at: clangd/index/MemIndex.h:23
 public:
-  /// \brief (Re-)Build index for `Symbols`. All symbol pointers must remain
-  /// accessible as long as `Symbols` is kept alive.
-  void build(std::shared_ptr<std::vector<const Symbol *>> Symbols);
+  using OccurrenceMap =
+      llvm::DenseMap<SymbolID, std::vector<const SymbolOccurrence *>>;
----------------
can you add an explanation to this?


================
Comment at: clangd/index/Merge.cpp:94
+    // instead.
+    llvm::DenseSet<llvm::StringRef> DynamicIndexFileURIs;
+    Dynamic->findOccurrences(Req, [&](const SymbolOccurrence &O) {
----------------
Unfortunately this is not safe, the backing strings may not live long enough.
This should be a StringSet (by value) instead.


================
Comment at: clangd/index/Merge.cpp:100
+    Static->findOccurrences(Req, [&](const SymbolOccurrence &O) {
+      if (DynamicIndexFileURIs.find(O.Location.FileURI) !=
+          DynamicIndexFileURIs.end())
----------------
or just DynamicIndexFileURIs.count(O.Location.FileURI) and rely on the implicit conversion to bool


================
Comment at: clangd/index/SymbolCollector.cpp:227
 
+SymbolOccurrenceKind ToOccurrenceKind(index::SymbolRoleSet Roles) {
+  SymbolOccurrenceKind Kind;
----------------
toOccurrenceKind

isn't this just 

`return Roles & AllOccurrenceKinds` with some casts?
(I'd put AllOccurrenceKinds into the header, but it could also be a constant here)


================
Comment at: clangd/index/SymbolCollector.h:120
+
+  using DeclOccurrence = std::pair<SourceLocation, index::SymbolRoleSet>;
+  llvm::DenseMap<const NamedDecl*, std::vector<DeclOccurrence>> DeclOccurrences;
----------------
move these up next to ReferencedDecls and ReferencedMacros so the comment applies to them too?

Similarly, move SymbolOccurrenceSlab next to the SymbolSlab::Builder? These have strong parallels.


================
Comment at: clangd/index/SymbolCollector.h:122
+  llvm::DenseMap<const NamedDecl*, std::vector<DeclOccurrence>> DeclOccurrences;
+  // All symbol occurrences collected from the AST, assembled on finish().
+  // Only symbols declared in preamble (from #inclues) and references from the
----------------
I'm not sure this works - IIRC can use a SymbolCollector for multiple TUs, with finish() called at the end of each one.
I think you need to (incrementally) build in finish(), and freeze in takeOccurrences().


================
Comment at: unittests/clangd/TestTU.h:41
 
+  static TestTU withAllCode(llvm::StringRef HeaderCode, llvm::StringRef Code,
+                            llvm::StringRef Filename = "") {
----------------
We've avoided adding this in the past because it's less readable. Please assign the fields separately instead.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D51279