[PATCH] D91122: [clangd] Call hierarchy (XRefs layer, incoming calls)

[Nathan Ridge via Phabricator via cfe-commits]

nridge added inline comments.


================
Comment at: clang-tools-extra/clangd/XRefs.h:109
+/// Get call hierarchy information at \p Pos.
+llvm::Optional<std::vector<CallHierarchyItem>>
+prepareCallHierarchy(ParsedAST &AST, Position Pos, const SymbolIndex *Index,
----------------
kadircet wrote:
> nridge wrote:
> > kadircet wrote:
> > > what's the distinction between empty and none return values ? (same for others)
> > Generally speaking, a `None` result means "the input was not recognized in some way", while an empty vector result means "there are no results for this input".
> > 
> > For `prepareCallHierarchy`, the inputs encode a source location, and the operation asks "give me `CallHierarchyItem`s for suitable declarations (i.e. functions) at this location. So, `None` means "the source location could not be recognized" (`sourceLocationInMainFile` failed), while an empty result means "there are no declarations of functions at this location".
> > 
> > For `incomingCalls` and `outgoingCalls`, the inputs encode a declaration of a function, and the operation asks "give me its callers / callees". So, a `None` means "could not locate the function (i.e. symbol)", while an empty result means "this function has no callers / callees".
> > Generally speaking, a None result means "the input was not recognized in some way", while an empty vector result means "there are no results for this input".
> 
> Makes sense, but that sounds like an implementation detail. I don't see any difference between the two from caller's point of view. Even the logging happens inside the implementation. As for interpreting llvm::None by the caller, i believe it is subject to change, so unless we return an Expected, there's not much value in returning an Optional, and even in such a case, caller probably can't do much but propagate the error (maybe also try with Pos+/-1, but usually that's handled within the XRefs as well).
> 
> Returning an empty container is also coherent with rest of the APIs we have in XRefs.h.
> 
> So I believe we should drop the optionals here.
The protocol [specifies](https://microsoft.github.io/language-server-protocol/specifications/specification-3-16/#textDocument_prepareCallHierarchy) return types of `CallHierarchyItem[] | null` for `prepareCallHierarchy` and `CallHierarchyIncomingCall[] | null` for `incomingCalls`.

The `| null` in there suggests that the protocol intends for there to be a semantic difference  between an empty array and null, and that clients may want to do things differently in the two caes (e.g. show an "unable to compute call hierarchy" error dialog vs. show an emty tree).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D91122