[PATCH] D72874: [clangd] Add a textual fallback for go-to-definition

Sun Mar 1 16:50:24 PST 2020

nridge added a comment.

I've addressed some of the review comments, with a view to getting something minimal we can land, and improve on in follow-up changes.

Mostly, I focused on the suggestions which reduce the number of results. I've left other suggestions which increase the number of results (e.g. handling non-indexed symbols) for follow-ups.

In D72874#1831977 <https://reviews.llvm.org/D72874#1831977>, @sammccall wrote:

> - only trigger when there's *some* positive signal for the word.
>   - Markup like quotes/backticks/brackets/`\p`
>   - weird case like `lowerCamel`, `UpperCamel`, `CAPS`, `mid-sentence Capitalization`, `under_scores`.
>   - use of the word as a token in nearby code (very close if very short, anywhere in file if longer)
>   - (maybe you want to support `ns::Qualifiers`?)

I currently handle `lowerCamel`, `UpperCamel`, `CAPS`, and `under_scores`. I've left the others as follow-ups.

> - post-filter aggressively - only return exact name matches (I think including case).

Done.

> - call `fuzzyFind` directly and set `ProximityPath`

Done.

> as well as the enclosing scopes from lexing. For extra strictness consider AnyScope=false

I haven't done this yet, do you think it's important for an initial landing?

If so, could you mention what API you had in mind for determining "enclosing scopes from lexing"?

I had in mind using something like `SelectionTree` and collecting any `RecordDecl`s or `NamespaceDecl`s on the path from the common ancestor to the TU, but that's technically not "from lexing", so perhaps you have something else in mind.

> - if you get more than 3 results, and none from current file, maybe don't return anything, as confidence is too low. Or try a stricter query...

I implemented this, but my testing shows this causes a lot of results for class names to be excluded. The reason appears to be that `fuzzyFind()` returns the class and each of its constructors as distinct results, so if a class has more than two constructors, we'll have more than 3 results (and typically the class is declared in a different file).

Should we try to handle this case specifically (collapse a class name and its construtors to a single result), or should we reconsider this filtering criterion? It's not exactly clear to me what sort of bad behaviour it's intended to weed out.

> - handle the most common case of non-indexable symbols (local symbols) by running the query against the closest occurrence of the token in code.

I've left this as a follow-up.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72874