[PATCH] D44305: [clangd] Add an interface that finds symbol by SymbolID in SymbolIndex.

[Sam McCall via Phabricator via cfe-commits]

sammccall accepted this revision.
sammccall added inline comments.
This revision is now accepted and ready to land.


================
Comment at: clangd/index/Index.h:253
+struct LookupRequest {
+  std::set<SymbolID> IDs;
+};
----------------
nit: DenseSet? we already have the traits


================
Comment at: clangd/index/Index.h:274
+  /// The returned symbol must be deep-copied if it's used outside Callback.
+  /// Returns true if at least one matched symbol was found.
+  virtual bool
----------------
is "at least one matched symbol was found" so useful that we need to provide it in a second way (rather than just invoking the callback at least once)?

Having both fuzzyFind and lookup return bool (but with very different semantics) seems confusing, and I suspect void would be fine here.


================
Comment at: clangd/index/Merge.cpp:67
+      if (!Sym)
+        B.insert(S);
+      else
----------------
this could just be callback(S)


================
Comment at: clangd/index/Merge.cpp:69
+      else
+        B.insert(mergeSymbol(*Sym, S, &Scratch));
+    });
----------------
This could also just be callback(mergeSymbol(...)), if we keep track of the IDs we've emitted.
This way the slab would only have symbols from the dynamic index.

This could be just:
 - before the static lookup, make a copy `RemainingIDs = Req.IDs`
 - in the static callback, remove the id from RemainingIDs
 - after the static lookup, loop through RemainingIDs, look up symbol in dynamic slab, emit

This avoids:
 - copying the static index results (which tend to be more numerous)
 - build() on the slab builder, which isn't cheap


================
Comment at: unittests/clangd/CodeCompleteTests.cpp:693
+  bool
+  lookup(const LookupRequest & /*Req*/,
+         llvm::function_ref<void(const Symbol &)> /*Callback*/) const override {
----------------
why comment out the param names here? can't we just either include or omit them?


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D44305