[PATCH] D54799: [clangd][WIP] textDocument/CursorInfo method

[Haojian Wu via Phabricator via cfe-commits]

hokein added inline comments.


================
Comment at: XRefs.cpp:95
   Preprocessor &PP;
+  const bool StopOnFirstDeclFound;
 
----------------
sammccall wrote:
> jkorous wrote:
> > sammccall wrote:
> > > Please don't do this - it's inconsistent with the other XRefs features.
> > > (If for some reason you *really* want just one result, then this is easy to do on the client side if we get the API right)
> > Sure, let's discuss. I probably got confused by your suggestion of using `getSymbolAtPosition()` in https://reviews.llvm.org/D54529#1299531.
> > 
> > Do I understand it correctly that you mean visiting all declarations and selecting the single explicit one in `getSymbolInfo()`?
> > Or do you actually mean change of interface such as this? `llvm::Optional<SymbolDetails> getSymbolInfo()` -> `std::vector<SymbolDetails> getSymbolInfo()`.
> > Is my assumption that there could be only single explicit declaration for any given position correct in the first place?
> I looked into this, and the multiple-symbols is more of a mess than I thought it was.
> 
> At a high level: I think it's important that various features that use "symbol under cursor" are consistent. If you do defs or refs or highlights at a point, you should be querying the same set of symbols. And the same goes for anything that starts with a symbolInfo call.
> 
> Unfortunately the way the sorting was introduced in rL341463 meant the existing things are not quite consistent, which confuses things a bit.
> 
> But at a high level:
>  - the status quo is that findDefinition/findRefs/highlights deal with multiple symbols. So yes, `symbolInfo` should return a `vector<SymbolDetails>` as things stand.
>  - the multiple-symbols situation is a bit surprising and maybe not that useful. It might be possible to change this behavior. It should be changed for all methods (i.e. either we land this patch with `vector<SymbolDetails>` and then (breaking) change it to `Optional<SymbolDetails>` later, or we remove the multiple-symbols behavior first.
>  - I don't know what the deal is with "explicit". The cases with multiple symbols are already obscure, cases with multiple explicit symbols are probably even rarer if they exist. But I don't think it's particularly safe to assume they don't. And getSymbolInfo really shouldn't be adding its own unique logic around this.
> 
> @hokein @ilya-biryukov We should chat about the historical context here.
I have the same question when I first touched this code :) 
IIRC, we want all symbols in GoToDef (as we can show multiple symbols to clients). Unfortunately, there are some inconsistencies between `def` (return all), `ref` (only return explicit symbols), `hover` (return the first one).

I agree that most cases are single-symbol, but the typical case with multiple-symbols is below. Personally, I'm +1 on removing the multiple-symbols behavior, it can simplify many things...

```
namespace ns {
void foo(int);
void foo(double);
}
using ns::f^oo;
```


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D54799