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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
