[PATCH] D91930: [clangd] Implement textDocument/codeLens

Wed Nov 25 04:30:48 PST 2020

kadircet added a comment.

Thanks for the explanations. I've also had some discussions with @sammccall about this and would like to summarize them.

There are two important things to keep in mind:

- Collection is performed for the whole document.
- Resolve requests cannot fail. (there's no `null` for the return type)

So this forces us to be really certain about existence of codelenses during collection time, while making sure latency is low enough for clangd to not feel sluggish, as this needs to be served after inactivity (i.e. not typing for a while).
Hence first of all we can't surface these lenses for all kinds of symbols, limiting the scope to top-level symbols as you do in this patch sounds like a sweet-spot.

But it is not enough on its own, for example we are still performing multiple index queries per symbol to figure out derived classes/methods, which doesn't scale well to big TUs, especially in presence of remote-index where each query can have considerable RTT.
So we need to ensure total number of index queries are kept to a minimum, possibly by batching all of the queries and performing them at once.
This requires significant changes to current implementation, but is really a must to ensure scalability of the feature. WDYT about evolving the patch in such a direction?

Another point around the usefulness of the lenses in this patch; The information we can deduce using only the AST doesn't seem to be useful.
Particularly:

- for bases, they are available next to the cursor and user can do go-to-definition on them.
- for overridden methods, most of the time there are 0 or 1, and again they keyword `override` is usually an indicator of that (moreover users can do go-to-def on that, but it is not as visible).

So it might be better to just drop them to not clutter the UI.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D91930