nridge marked an inline comment as done. nridge added inline comments.
================ Comment at: clang-tools-extra/clangd/XRefs.cpp:1649 + // into the same CallHierarchyIncomingCall. + llvm::DenseMap<SymbolID, CallHierarchyIncomingCall> ResultMap; + // We can populate the keys of ResultMap, and the FromRanges fields of ---------------- kadircet wrote: > nit: `s/ResultMap/CallsIn` ? > > I would also suggest changing value type to `SmallVector<Range, 1>` that way > rather than having a "partially filled" IncomingCall objects in the middle, > you can create valid ones directly within the lookup. I went with `std::vector<Range>` as we eventually need that for the final result, and we can move it from one place to another. ================ 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: > > > 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). > yes, that's indeed a possibility, and the same goes for other features like > textDocument/{definition,declaration,references}. AFAICT, we don't signal the > difference between "no results" and "something went wrong" for those, and > haven't received any complaints yet. So I'd rather keep them coherent, and > replace all of them if we see that there are clients taking different actions > for real. Ok, removed the Optional for consistency with other requests. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D91122/new/ https://reviews.llvm.org/D91122 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
