arphaman added a comment. In https://reviews.llvm.org/D40562#941753, @ilya-biryukov wrote:
> In https://reviews.llvm.org/D40562#941570, @arphaman wrote: > > > I'm not actually 100% sure, but I would imagine that this one of the > > reasons, yes. It would be nice to improve the cache to have things like > > namespace-level `Decl`, although how will lookup work in that case? Btw, do > > you think the cache can be reused in clangd as well? > > > As Eric mentioned, we are planning to have project-global completion for > namespace-level Decls (to have completion items not #included in the current > file and add the #include directive properly). So the cache is probably not > that useful to clangd long-term. Interesting, thanks! Will this be something that clients of clangd can opt-out from? Or at least configure certain aspects of the behaviour? > For proper lookup in the cache that include all namespace-level Decls I'd go > with tweaking `LookupVisibleDecls` so that it does not deserialize everything > from the preamble, but rather provides a list of scopes that we need to get > completion items from. Though sounds simple, it may be a non-trivial change > and we shouldn't probably pursue it as part of this change. > (We'll probably need it for clangd too). > > In https://reviews.llvm.org/D40562#941735, @ioeric wrote: > >> I took a quick look at the completion cache and lookup code. I think the >> completion cache also assumes that top-level decls are only TU-level decls, >> and this assumption seems to be also built into the lookup code. At this >> point, I am inclined to add a separate completion option for what I want >> (`IgnoreDeclsInTUOrNamespaces`?). Regarding cache in clangd, I think it >> might be useful short-term, when we still use Sema's global code completion, >> but long term, we would use symbols from clangd's indexes, so the cache >> would not be useful anymore. > > > +1 for having a separate flag. Maybe call it `IncludeNamespaceLevelDecls` > instead? (I'd say TU is also a (global) namespace from completion's point of > view). I agree with the new flag as well. I would also like to see a followup patch where this change is used before this is committed. https://reviews.llvm.org/D40562 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
