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