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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
