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

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet added a comment.

mostly LG, i haven't looked at the tests yet though.



================
Comment at: clang-tools-extra/clangd/XRefs.cpp:1344
+  auto Result = declToHierarchyItem<TypeHierarchyItem>(ND);
+  if (Result) {
+    Result->deprecated = ND.isDeprecated();
----------------
nit: redundant braces


================
Comment at: clang-tools-extra/clangd/XRefs.cpp:1353
+  auto Result = declToHierarchyItem<CallHierarchyItem>(ND);
+  if (Result) {
+    if (ND.isDeprecated())
----------------
nit: merge the two conditions and drop the braces.


================
Comment at: clang-tools-extra/clangd/XRefs.cpp:1361
+template <typename HierarchyItem>
+static Optional<HierarchyItem> symbolToHierarchyItem(const Symbol &S,
+                                                     PathRef TUPath) {
----------------
can you qualify `Optional` here and elsewhere?


================
Comment at: clang-tools-extra/clangd/XRefs.cpp:1381
 
-  return std::move(THI);
+  return std::move(HI);
+}
----------------
nit: drop `std::move`


================
Comment at: clang-tools-extra/clangd/XRefs.cpp:1387
+  auto Result = symbolToHierarchyItem<TypeHierarchyItem>(S, TUPath);
+  if (Result) {
+    Result->deprecated = (S.Flags & Symbol::Deprecated);
----------------
nit: redundant braces


================
Comment at: clang-tools-extra/clangd/XRefs.cpp:1396
+  auto Result = symbolToHierarchyItem<CallHierarchyItem>(S, TUPath);
+  if (Result) {
+    if (S.Flags & Symbol::Deprecated) {
----------------
nit: again merge conditions and drop braces


================
Comment at: clang-tools-extra/clangd/XRefs.cpp:1620
+    if (auto CHI = declToCallHierarchyItem(*Decl))
+      Result.push_back(*std::move(CHI));
+  }
----------------
`Results.emplace_back(std::move(*CHI))` ?


================
Comment at: clang-tools-extra/clangd/XRefs.cpp:1629
+    return llvm::None;
+  Expected<SymbolID> ID = SymbolID::fromStr(Item.data);
+  if (!ID) {
----------------
nit: `auto ID`


================
Comment at: clang-tools-extra/clangd/XRefs.cpp:1646
+  Request.Filter = RefKind::Reference;
+  // Initially store the results in a map keyed by SymbolID.
+  // This allows us to group different calls with the same caller
----------------
... by SymbolID of the caller.


================
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
----------------
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.


================
Comment at: clang-tools-extra/clangd/XRefs.cpp:1660
+    }
+    auto It = ResultMap.find(R.Container);
+    if (It == ResultMap.end())
----------------
nit: `auto It = ResultMap.try_emplace(R.Container, CallHierarchyIncomingCall{}).first`


================
Comment at: clang-tools-extra/clangd/XRefs.cpp:1671
+    auto It = ResultMap.find(Caller.ID);
+    if (It != ResultMap.end()) {
+      if (auto CHI = symbolToCallHierarchyItem(Caller, Item.uri.file()))
----------------
why don't we assert this instead?


================
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,
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D91122