[PATCH] D42049: [clangd] Merge results from static/dynamic index.

[Sam McCall via Phabricator via cfe-commits]

sammccall added inline comments.


================
Comment at: clangd/index/Merge.cpp:29
+     //    b) if it's in the dynamic slab, merge it and yield the result
+     //  3) now yield all the dynamic symbols we haven't processed.
+     bool More = false; // We'll be incomplete if either source was.
----------------
hokein wrote:
> Maybe add another FIXME, saying filer out symbols in static index, but are removed in dynamic index?
Done (function-level comment, because it's not a localized fix)


================
Comment at: clangd/index/Merge.cpp:34
+         Dynamic->fuzzyFind(Ctx, Req, [&](const Symbol &S) { DynB.insert(S); });
+     SymbolSlab Dyn = std::move(DynB).build();
+
----------------
hokein wrote:
> IIUC, `Dyn` is a local variable, which holds all the underlying data of `Symbol`, the `Symbol` returned in the callback of `fuzzyFind` would be invalid after the `fuzzyFind(..)` function call is finished.
> 
> Our previous assumption is that `Symbol` is valid as long as the `SymbolIndex` exists?
> Our previous assumption is that Symbol is valid as long as the SymbolIndex exists?

No, it's only valid for the callback. Valid for the life of the index would make life hard for remote indexes.
Added documentation to SymbolIndex.


================
Comment at: clangd/index/Merge.cpp:75
+    if (S.Detail->Documentation == "")
+      S.Detail->Documentation = R.Detail->Documentation;
+  }
----------------
hokein wrote:
> Don't we need ` CompletionDetail`?
Sure. I was assuming it wasn't optional (should be present and match if USR matches), but i'm not 100% sure.


================
Comment at: clangd/index/Merge.cpp:75
+    if (S.Detail->Documentation == "")
+      S.Detail->Documentation = R.Detail->Documentation;
+  }
----------------
sammccall wrote:
> hokein wrote:
> > Don't we need ` CompletionDetail`?
> Sure. I was assuming it wasn't optional (should be present and match if USR matches), but i'm not 100% sure.
This was a bug. It'd be caught by marking Symbol::Detail const. Added a TODO.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D42049