sammccall added a comment.

In D75479#1908774 <https://reviews.llvm.org/D75479#1908774>, @nridge wrote:

> This method only returns `Bar::uniqueMethodName()` because it's closer in 
> terms of distance


Yeah, missing data is definitely bad:

- not finding results makes the feature feel flaky or unreliable
- finding *partial* results can be misleading and makes the feature 
untrustworthy

> Is that perhaps a reason to adjust the order in which we try the approaches 
> (i.e. use this one as a fallback if index lookup fails)?

Well, it's even easier to construct examples where the index approach is 
missing some data (most symbols are not indexable) or gives wrong results (name 
collisions are common) which is probably worse.
So I'm not convinced by this that the index is more accurate.

> Or should we try to allow multiple results with this approach as well?

It's an interesting idea, but two immediate risks:

- it's clearly worse for things like comment navigation local variables
- it helps your example only quite narrowly: all the targets need to be in the 
current file

We have several building blocks and various ways for how to put them together.
I think we should go back to the use cases (e.g. dependent code) and work out 
how each is best served, my intuition is that pretty different. Discussing how 
the building blocks should relate to each other in the general case may run in 
circles a bit :-) I'll make a proposal on 
https://github.com/clangd/clangd/issues/241 for a bit more visibility.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D75479/new/

https://reviews.llvm.org/D75479



_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Reply via email to