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

Sam McCall via Phabricator via cfe-commits cfe-commits at lists.llvm.org
Tue Mar 13 10:22:15 PDT 2018

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?

  rCTE Clang Tools Extra


More information about the cfe-commits mailing list