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

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet added inline comments.


================
Comment at: clang-tools-extra/clangd/XRefs.cpp:1317
-static Optional<TypeHierarchyItem>
-symbolToTypeHierarchyItem(const Symbol &S, const SymbolIndex *Index,
-                          PathRef TUPath) {
----------------
that's a great change to have, but maybe leave it out of this patch?


================
Comment at: clang-tools-extra/clangd/XRefs.cpp:1544
+static llvm::Optional<CallHierarchyItem>
+declToCallHierarchyItem(ASTContext &Ctx, const NamedDecl &ND) {
+  auto &SM = Ctx.getSourceManager();
----------------
can we rather have a template like:
```
template <typename HierarchyItem>
HierarchyItem fromDecl(const NamedDecl &ND) {
HierarchyItem Result;
auto &Ctx = ND.getASTContext();
....
return Result;
}
```

To merge this one with the `declToCallHierarchy` ? The only separate bit seems to be around `deprecated`. They can be set explicitly outside. (looks like we forgot to add the SymbolTag for deprecated here)


================
Comment at: clang-tools-extra/clangd/XRefs.cpp:1604
+  if (!Loc) {
+    llvm::consumeError(Loc.takeError());
+    return llvm::None;
----------------
can we log the error instead?


================
Comment at: clang-tools-extra/clangd/XRefs.cpp:1609
+  for (const NamedDecl *Decl : getDeclAtPosition(AST, *Loc, {})) {
+    if (Decl->isFunctionOrFunctionTemplate()) {
+      if (auto CHI = declToCallHierarchyItem(AST.getASTContext(), *Decl))
----------------
nit: early exit `if (!FuncOrFuncTempl)continue;`


================
Comment at: clang-tools-extra/clangd/XRefs.cpp:1617
+
+llvm::Optional<CallHierarchyItem> symbolToCallHierarchyItem(const Symbol &S,
+                                                            PathRef TUPath) {
----------------
this can be merged with the typehierarchy implementation using a similar template trick mentioned above.


================
Comment at: clang-tools-extra/clangd/XRefs.cpp:1621
+  if (!Loc) {
+    log("Type hierarchy: {0}", Loc.takeError());
+    return llvm::None;
----------------
let's `elog` instead. I think just saying `Failed to convert symbol to hierarchy item: {0}` should be fine in the merged implementation.


================
Comment at: clang-tools-extra/clangd/XRefs.cpp:1648
+  Expected<SymbolID> ID = SymbolID::fromStr(*Item.Data);
+  if (!ID)
+    return llvm::None;
----------------
the error needs to be consumed (preferably by logging)


================
Comment at: clang-tools-extra/clangd/XRefs.cpp:1652
+  Request.IDs.insert(*ID);
+  // FIXME: Perhaps we should be even more specific and introduce a
+  // RefKind for calls, and use that?
----------------
i suppose the main problem is we might have false positives:

```
void foo();
void bar() {
 funcTakingFunc(&foo);
}
```

bar looks like it is calling foo, but it isn't. but i think it is useful, and probably the only place we can point out a potential indirect call without crazy static analysis. So even if we had separate kinds for call and mere references, i believe we would still surface both. Hence, can we just mention this caveat rather than putting a fixme here?


================
Comment at: clang-tools-extra/clangd/XRefs.cpp:1660
+  Index->refs(Request, [&](const Ref &R) {
+    if (auto Loc = indexToLSPLocation(R.Location, Item.Uri.file())) {
+      LookupRequest Lookup;
----------------
early exit, also the error needs to be consumed


================
Comment at: clang-tools-extra/clangd/XRefs.cpp:1663
+      Lookup.IDs.insert(R.Container);
+      Index->lookup(Lookup, [&](const Symbol &Caller) {
+        // See if we already have a CallHierarchyIncomingCall for this caller.
----------------
instead of performing a lookup per reference, can we just accumulate the symbolids and do a single lookup after finding all the unique containers?


================
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,
----------------
what's the distinction between empty and none return values ? (same for others)


================
Comment at: clang-tools-extra/clangd/XRefs.h:110
+llvm::Optional<std::vector<CallHierarchyItem>>
+prepareCallHierarchy(ParsedAST &AST, Position Pos, const SymbolIndex *Index,
+                     PathRef TUPath);
----------------
why do we need the index in this one?


================
Comment at: clang-tools-extra/clangd/XRefs.h:114
+llvm::Optional<std::vector<CallHierarchyIncomingCall>>
+incomingCalls(const CallHierarchyItem &Item, const SymbolIndex *Index);
+
----------------
In theory AST will have more up-to-date information about ranges/occurrences within the current file. As dynamic index for it might be stale. (the same goes for typehierarchy, we've just forgot about it at the time).

But going from SymbolID to an AST node isn't cheap, especially when the declaration isn't inside the main file. So I suppose it is OK to move with an index only implementation for now, but we should leave a FIXME to remind us about the situation.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D91122