malaperle added inline comments.
================ Comment at: clangd/index/Index.h:134 + virtual bool + fuzzyFind(Context &Ctx, const FuzzyFindRequest &Req, + std::function<void(const Symbol &)> Callback) const = 0; ---------------- sammccall wrote: > ioeric wrote: > > malaperle wrote: > > > ioeric wrote: > > > > malaperle wrote: > > > > > I think a more generic std::function would be useful, similar to the > > > > > indexstore's filter > > > > > ``` > > > > > bool searchSymbols(llvm::function_ref<bool(IndexRecordSymbol, bool > > > > > &stop)> filter, > > > > > llvm::function_ref<void(IndexRecordSymbol)> > > > > > receiver) > > > > > ``` > > > > Do you have an use case in mind which requires different filtering? > > > > This could probably be a separate interface. > > > > > > > > I think the behavior of fuzzy finding is well-defined, and how > > > > filtering is done is an implementation detail. User-specified filter > > > > might make implementation less flexible, especially for large indexes > > > > that are not managed in memory. > > > For example > > > - Searching by USR > > > - By non-qualified name (useful also for Open Workspace Symbol) > > > - Most likely other filters, hmm, by Symbol kind, language? > > > - No filter, i.e. retrieve *all* symbols. Useful mainly for development > > > and to get index statistics > > > > > > There could also be a boolean in the callback return value (or the filter > > > callback) to limit the number of returned values. > > > I haven't tried locally but it looks like it would be pretty doable to > > > change the FuzzyFindRequest into a std::function. > > > A big disadvantage of having just one method though is that is would be > > > difficult for an implementation to optimize for a specific type of query. > > > For example, if you search by non-qualified name, an implementation could > > > use a mapping of non-qualified-name-to-USR but it would be difficult to > > > know what to do given only the std::function/filter passed by argument. > > > In that sense, perhaps it is better to have multiple methods for each use > > > cases. Or perhaps some enum for each kind of query. What do you think? > > Adding new interfaces is an option. It should also be easy to extend the > > existing interface to cover more use cases. > > > > > Searching by USR > > We definitely want an interface for this. I can see this being useful for > > Goto-definition and other features. > > > By non-qualified name (useful also for Open Workspace Symbol) > > I think `fuzzyFind` can support this, with fuzzy matching optionally turned > > off if preferred. > > > Most likely other filters, hmm, by Symbol kind, language? > > Similarly, this could be achieved by extending the find request to include > > more filters e.g. symbol kind. > > > No filter, i.e. retrieve *all* symbols. Useful mainly for development and > > > to get index statistics > > This can be done today by setting an empty fuzzy filter. > > > > > As Eric said, we should be able to add more options here. > But why are we reluctant to add an arbitrary std::function, which is more > elegant and flexible? > > - it essentially forbids remote indexes. Because functions aren't > serializable, the only possible implementation is to send all the symbols to > clangd. For our main codebase, this is would be ~1G for just qnames, and a > lot more for full symbols. > - A slightly more subtle disadvantage is because functions aren't > serializable, it's harder to debug them - you can't just log the query. > - filtering functions are easy to implement (so can be distributed over > callsites), but scoring functions are hard (so should be more centralized). > They tend to operate on the same data. It's not obvious how to achieve this > with a filter API. Thanks a lot, that makes sense. I do think it would be good, perhaps for other kinds of queries, that the query could be stopped halfway by having the Callback return a bool. ``` std::function<void(const Symbol &)> Callback) const ``` to ``` std::function<bool(const Symbol &)> Callback) const ``` But this can be revisited once we have other options. Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D40548 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
