[clangd] Support outgoing calls in call hierarchy #77556
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
@llvm/pr-subscribers-clang-tools-extra @llvm/pr-subscribers-clangd Author: Nathan Ridge (HighCommander4) ChangesPatch is 49.27 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/77556.diff 23 Files Affected:
diff --git a/clang-tools-extra/clangd/ClangdLSPServer.cpp b/clang-tools-extra/clangd/ClangdLSPServer.cpp
index a87da252b7a7e9..482dae521fbcd5 100644
--- a/clang-tools-extra/clangd/ClangdLSPServer.cpp
+++ b/clang-tools-extra/clangd/ClangdLSPServer.cpp
@@ -1373,6 +1373,12 @@ void ClangdLSPServer::onInlayHint(const InlayHintsParams &Params,
std::move(Reply));
}
+void ClangdLSPServer::onCallHierarchyOutgoingCalls(
+ const CallHierarchyOutgoingCallsParams &Params,
+ Callback<std::vector<CallHierarchyOutgoingCall>> Reply) {
+ Server->outgoingCalls(Params.item, std::move(Reply));
+}
+
void ClangdLSPServer::applyConfiguration(
const ConfigurationSettings &Settings) {
// Per-file update to the compilation database.
@@ -1654,6 +1660,7 @@ void ClangdLSPServer::bindMethods(LSPBinder &Bind,
Bind.method("typeHierarchy/subtypes", this, &ClangdLSPServer::onSubTypes);
Bind.method("textDocument/prepareCallHierarchy", this, &ClangdLSPServer::onPrepareCallHierarchy);
Bind.method("callHierarchy/incomingCalls", this, &ClangdLSPServer::onCallHierarchyIncomingCalls);
+ Bind.method("callHierarchy/outgoingCalls", this, &ClangdLSPServer::onCallHierarchyOutgoingCalls);
Bind.method("textDocument/selectionRange", this, &ClangdLSPServer::onSelectionRange);
Bind.method("textDocument/documentLink", this, &ClangdLSPServer::onDocumentLink);
Bind.method("textDocument/semanticTokens/full", this, &ClangdLSPServer::onSemanticTokens);
diff --git a/clang-tools-extra/clangd/ClangdLSPServer.h b/clang-tools-extra/clangd/ClangdLSPServer.h
index 79579c22b788a9..50a0a7e43a4436 100644
--- a/clang-tools-extra/clangd/ClangdLSPServer.h
+++ b/clang-tools-extra/clangd/ClangdLSPServer.h
@@ -153,6 +153,9 @@ class ClangdLSPServer : private ClangdServer::Callbacks,
void onCallHierarchyIncomingCalls(
const CallHierarchyIncomingCallsParams &,
Callback<std::vector<CallHierarchyIncomingCall>>);
+ void onCallHierarchyOutgoingCalls(
+ const CallHierarchyOutgoingCallsParams &,
+ Callback<std::vector<CallHierarchyOutgoingCall>>);
void onClangdInlayHints(const InlayHintsParams &,
Callback<llvm::json::Value>);
void onInlayHint(const InlayHintsParams &, Callback<std::vector<InlayHint>>);
diff --git a/clang-tools-extra/clangd/ClangdServer.cpp b/clang-tools-extra/clangd/ClangdServer.cpp
index 6fb2641e8793db..5c0303c9bf9448 100644
--- a/clang-tools-extra/clangd/ClangdServer.cpp
+++ b/clang-tools-extra/clangd/ClangdServer.cpp
@@ -880,6 +880,15 @@ void ClangdServer::inlayHints(PathRef File, std::optional<Range> RestrictRange,
WorkScheduler->runWithAST("InlayHints", File, std::move(Action), Transient);
}
+void ClangdServer::outgoingCalls(
+ const CallHierarchyItem &Item,
+ Callback<std::vector<CallHierarchyOutgoingCall>> CB) {
+ WorkScheduler->run("Outgoing Calls", "",
+ [CB = std::move(CB), Item, this]() mutable {
+ CB(clangd::outgoingCalls(Item, Index));
+ });
+}
+
void ClangdServer::onFileEvent(const DidChangeWatchedFilesParams &Params) {
// FIXME: Do nothing for now. This will be used for indexing and potentially
// invalidating other caches.
diff --git a/clang-tools-extra/clangd/ClangdServer.h b/clang-tools-extra/clangd/ClangdServer.h
index a416602251428b..3319c44e75d3fc 100644
--- a/clang-tools-extra/clangd/ClangdServer.h
+++ b/clang-tools-extra/clangd/ClangdServer.h
@@ -288,6 +288,10 @@ class ClangdServer {
void incomingCalls(const CallHierarchyItem &Item,
Callback<std::vector<CallHierarchyIncomingCall>>);
+ /// Resolve outgoing calls for a given call hierarchy item.
+ void outgoingCalls(const CallHierarchyItem &Item,
+ Callback<std::vector<CallHierarchyOutgoingCall>>);
+
/// Resolve inlay hints for a given document.
void inlayHints(PathRef File, std::optional<Range> RestrictRange,
Callback<std::vector<InlayHint>>);
diff --git a/clang-tools-extra/clangd/XRefs.cpp b/clang-tools-extra/clangd/XRefs.cpp
index 5f41f788a69393..8095a36b7fb4ec 100644
--- a/clang-tools-extra/clangd/XRefs.cpp
+++ b/clang-tools-extra/clangd/XRefs.cpp
@@ -1700,6 +1700,7 @@ declToHierarchyItem(const NamedDecl &ND, llvm::StringRef TUPath) {
HierarchyItem HI;
HI.name = printName(Ctx, ND);
+ // FIXME: Populate HI.detail the way we do in symbolToHierarchyItem?
HI.kind = SK;
HI.range = Range{sourceLocToPosition(SM, DeclRange->getBegin()),
sourceLocToPosition(SM, DeclRange->getEnd())};
@@ -1751,6 +1752,7 @@ static std::optional<HierarchyItem> symbolToHierarchyItem(const Symbol &S,
}
HierarchyItem HI;
HI.name = std::string(S.Name);
+ HI.detail = (S.Scope + S.Name).str();
HI.kind = indexSymbolKindToSymbolKind(S.SymInfo.Kind);
HI.selectionRange = Loc->range;
// FIXME: Populate 'range' correctly
@@ -2303,6 +2305,63 @@ incomingCalls(const CallHierarchyItem &Item, const SymbolIndex *Index) {
return Results;
}
+std::vector<CallHierarchyOutgoingCall>
+outgoingCalls(const CallHierarchyItem &Item, const SymbolIndex *Index) {
+ std::vector<CallHierarchyOutgoingCall> Results;
+ if (!Index || Item.data.empty())
+ return Results;
+ auto ID = SymbolID::fromStr(Item.data);
+ if (!ID) {
+ elog("outgoingCalls failed to find symbol: {0}", ID.takeError());
+ return Results;
+ }
+ // In this function, we find outgoing calls based on the index only.
+ ContainedRefsRequest Request;
+ Request.ID = *ID;
+ // Initially store the ranges in a map keyed by SymbolID of the callee.
+ // This allows us to group different calls to the same function
+ // into the same CallHierarchyOutgoingCall.
+ llvm::DenseMap<SymbolID, std::vector<Range>> CallsOut;
+ // We can populate the ranges based on a refs request only. As we do so, we
+ // also accumulate the callee IDs into a lookup request.
+ LookupRequest CallsOutLookup;
+ Index->containedRefs(Request, [&](const auto &R) {
+ auto Loc = indexToLSPLocation(R.Location, Item.uri.file());
+ if (!Loc) {
+ elog("outgoingCalls failed to convert location: {0}", Loc.takeError());
+ return;
+ }
+ auto It = CallsOut.try_emplace(R.Symbol, std::vector<Range>{}).first;
+ It->second.push_back(Loc->range);
+
+ CallsOutLookup.IDs.insert(R.Symbol);
+ });
+ // Perform the lookup request and combine its results with CallsOut to
+ // get complete CallHierarchyOutgoingCall objects.
+ Index->lookup(CallsOutLookup, [&](const Symbol &Callee) {
+ // Filter references to only keep function calls
+ using SK = index::SymbolKind;
+ auto Kind = Callee.SymInfo.Kind;
+ bool NotCall = (Kind != SK::Function && Kind != SK::InstanceMethod &&
+ Kind != SK::ClassMethod && Kind != SK::StaticMethod &&
+ Kind != SK::Constructor && Kind != SK::Destructor &&
+ Kind != SK::ConversionFunction);
+ assert(!NotCall);
+
+ auto It = CallsOut.find(Callee.ID);
+ assert(It != CallsOut.end());
+ if (auto CHI = symbolToCallHierarchyItem(Callee, Item.uri.file()))
+ Results.push_back(
+ CallHierarchyOutgoingCall{std::move(*CHI), std::move(It->second)});
+ });
+ // Sort results by name of the callee.
+ llvm::sort(Results, [](const CallHierarchyOutgoingCall &A,
+ const CallHierarchyOutgoingCall &B) {
+ return A.to.name < B.to.name;
+ });
+ return Results;
+}
+
llvm::DenseSet<const Decl *> getNonLocalDeclRefs(ParsedAST &AST,
const FunctionDecl *FD) {
if (!FD->hasBody())
diff --git a/clang-tools-extra/clangd/XRefs.h b/clang-tools-extra/clangd/XRefs.h
index df91dd15303c18..247e52314c3f94 100644
--- a/clang-tools-extra/clangd/XRefs.h
+++ b/clang-tools-extra/clangd/XRefs.h
@@ -150,6 +150,9 @@ prepareCallHierarchy(ParsedAST &AST, Position Pos, PathRef TUPath);
std::vector<CallHierarchyIncomingCall>
incomingCalls(const CallHierarchyItem &Item, const SymbolIndex *Index);
+std::vector<CallHierarchyOutgoingCall>
+outgoingCalls(const CallHierarchyItem &Item, const SymbolIndex *Index);
+
/// Returns all decls that are referenced in the \p FD except local symbols.
llvm::DenseSet<const Decl *> getNonLocalDeclRefs(ParsedAST &AST,
const FunctionDecl *FD);
diff --git a/clang-tools-extra/clangd/index/Index.cpp b/clang-tools-extra/clangd/index/Index.cpp
index 7a0c23287db225..86dc6ed7633449 100644
--- a/clang-tools-extra/clangd/index/Index.cpp
+++ b/clang-tools-extra/clangd/index/Index.cpp
@@ -66,6 +66,11 @@ bool SwapIndex::refs(const RefsRequest &R,
llvm::function_ref<void(const Ref &)> CB) const {
return snapshot()->refs(R, CB);
}
+bool SwapIndex::containedRefs(
+ const ContainedRefsRequest &R,
+ llvm::function_ref<void(const ContainedRefsResult &)> CB) const {
+ return snapshot()->containedRefs(R, CB);
+}
void SwapIndex::relations(
const RelationsRequest &R,
llvm::function_ref<void(const SymbolID &, const Symbol &)> CB) const {
diff --git a/clang-tools-extra/clangd/index/Index.h b/clang-tools-extra/clangd/index/Index.h
index 047ce08e93e3ab..a193b1a191216a 100644
--- a/clang-tools-extra/clangd/index/Index.h
+++ b/clang-tools-extra/clangd/index/Index.h
@@ -77,6 +77,19 @@ struct RefsRequest {
bool WantContainer = false;
};
+struct ContainedRefsRequest {
+ /// Note that RefKind::Call just restricts the matched SymbolKind to
+ /// functions, not the form of the reference (e.g. address-of-function,
+ /// which can indicate an indirect call, should still be caught).
+ static const RefKind SupportedRefKinds = RefKind::Call;
+
+ SymbolID ID;
+ /// If set, limit the number of refers returned from the index. The index may
+ /// choose to return less than this, e.g. it tries to avoid returning stale
+ /// results.
+ std::optional<uint32_t> Limit;
+};
+
struct RelationsRequest {
llvm::DenseSet<SymbolID> Subjects;
RelationKind Predicate;
@@ -84,6 +97,14 @@ struct RelationsRequest {
std::optional<uint32_t> Limit;
};
+struct ContainedRefsResult {
+ /// The source location where the symbol is named.
+ SymbolLocation Location;
+ RefKind Kind = RefKind::Unknown;
+ /// The ID of the symbol which is referred to
+ SymbolID Symbol;
+};
+
/// Describes what data is covered by an index.
///
/// Indexes may contain symbols but not references from a file, etc.
@@ -141,6 +162,17 @@ class SymbolIndex {
virtual bool refs(const RefsRequest &Req,
llvm::function_ref<void(const Ref &)> Callback) const = 0;
+ /// Find all symbols that are referenced by a symbol and apply
+ /// \p Callback on each result.
+ ///
+ /// Results should be returned in arbitrary order.
+ /// The returned result must be deep-copied if it's used outside Callback.
+ ///
+ /// Returns true if there will be more results (limited by Req.Limit);
+ virtual bool containedRefs(
+ const ContainedRefsRequest &Req,
+ llvm::function_ref<void(const ContainedRefsResult &)> Callback) const = 0;
+
/// Finds all relations (S, P, O) stored in the index such that S is among
/// Req.Subjects and P is Req.Predicate, and invokes \p Callback for (S, O) in
/// each.
@@ -175,6 +207,9 @@ class SwapIndex : public SymbolIndex {
llvm::function_ref<void(const Symbol &)>) const override;
bool refs(const RefsRequest &,
llvm::function_ref<void(const Ref &)>) const override;
+ bool containedRefs(
+ const ContainedRefsRequest &,
+ llvm::function_ref<void(const ContainedRefsResult &)>) const override;
void relations(const RelationsRequest &,
llvm::function_ref<void(const SymbolID &, const Symbol &)>)
const override;
diff --git a/clang-tools-extra/clangd/index/MemIndex.cpp b/clang-tools-extra/clangd/index/MemIndex.cpp
index fe0ee873018b37..ff966529ae188a 100644
--- a/clang-tools-extra/clangd/index/MemIndex.cpp
+++ b/clang-tools-extra/clangd/index/MemIndex.cpp
@@ -9,6 +9,7 @@
#include "MemIndex.h"
#include "FuzzyMatch.h"
#include "Quality.h"
+#include "index/Index.h"
#include "support/Trace.h"
namespace clang {
@@ -85,6 +86,25 @@ bool MemIndex::refs(const RefsRequest &Req,
return false; // We reported all refs.
}
+bool MemIndex::containedRefs(
+ const ContainedRefsRequest &Req,
+ llvm::function_ref<void(const ContainedRefsResult &)> Callback) const {
+ trace::Span Tracer("MemIndex refersTo");
+ uint32_t Remaining = Req.Limit.value_or(std::numeric_limits<uint32_t>::max());
+ for (const auto &Pair : Refs) {
+ for (const auto &R : Pair.second) {
+ if (!static_cast<int>(ContainedRefsRequest::SupportedRefKinds & R.Kind) ||
+ Req.ID != R.Container)
+ continue;
+ if (Remaining == 0)
+ return true; // More refs were available.
+ --Remaining;
+ Callback({R.Location, R.Kind, Pair.first});
+ }
+ }
+ return false; // We reported all refs.
+}
+
void MemIndex::relations(
const RelationsRequest &Req,
llvm::function_ref<void(const SymbolID &, const Symbol &)> Callback) const {
diff --git a/clang-tools-extra/clangd/index/MemIndex.h b/clang-tools-extra/clangd/index/MemIndex.h
index fba2c1a7120a2b..8f390c5028dc4d 100644
--- a/clang-tools-extra/clangd/index/MemIndex.h
+++ b/clang-tools-extra/clangd/index/MemIndex.h
@@ -72,6 +72,10 @@ class MemIndex : public SymbolIndex {
bool refs(const RefsRequest &Req,
llvm::function_ref<void(const Ref &)> Callback) const override;
+ bool containedRefs(const ContainedRefsRequest &Req,
+ llvm::function_ref<void(const ContainedRefsResult &)>
+ Callback) const override;
+
void relations(const RelationsRequest &Req,
llvm::function_ref<void(const SymbolID &, const Symbol &)>
Callback) const override;
diff --git a/clang-tools-extra/clangd/index/Merge.cpp b/clang-tools-extra/clangd/index/Merge.cpp
index 8221d4b1f44405..aecca38a885b66 100644
--- a/clang-tools-extra/clangd/index/Merge.cpp
+++ b/clang-tools-extra/clangd/index/Merge.cpp
@@ -155,6 +155,40 @@ bool MergedIndex::refs(const RefsRequest &Req,
return More || StaticHadMore;
}
+bool MergedIndex::containedRefs(
+ const ContainedRefsRequest &Req,
+ llvm::function_ref<void(const ContainedRefsResult &)> Callback) const {
+ trace::Span Tracer("MergedIndex refersTo");
+ bool More = false;
+ uint32_t Remaining = Req.Limit.value_or(std::numeric_limits<uint32_t>::max());
+ // We don't want duplicated refs from the static/dynamic indexes,
+ // and we can't reliably deduplicate them because offsets may differ slightly.
+ // We consider the dynamic index authoritative and report all its refs,
+ // and only report static index refs from other files.
+ More |= Dynamic->containedRefs(Req, [&](const auto &O) {
+ Callback(O);
+ assert(Remaining != 0);
+ --Remaining;
+ });
+ if (Remaining == 0 && More)
+ return More;
+ auto DynamicContainsFile = Dynamic->indexedFiles();
+ // We return less than Req.Limit if static index returns more refs for dirty
+ // files.
+ bool StaticHadMore = Static->containedRefs(Req, [&](const auto &O) {
+ if ((DynamicContainsFile(O.Location.FileURI) & IndexContents::References) !=
+ IndexContents::None)
+ return; // ignore refs that have been seen from dynamic index.
+ if (Remaining == 0) {
+ More = true;
+ return;
+ }
+ --Remaining;
+ Callback(O);
+ });
+ return More || StaticHadMore;
+}
+
llvm::unique_function<IndexContents(llvm::StringRef) const>
MergedIndex::indexedFiles() const {
return [DynamicContainsFile{Dynamic->indexedFiles()},
diff --git a/clang-tools-extra/clangd/index/Merge.h b/clang-tools-extra/clangd/index/Merge.h
index b8a562b0df5d92..7441be6e57e854 100644
--- a/clang-tools-extra/clangd/index/Merge.h
+++ b/clang-tools-extra/clangd/index/Merge.h
@@ -38,6 +38,9 @@ class MergedIndex : public SymbolIndex {
llvm::function_ref<void(const Symbol &)>) const override;
bool refs(const RefsRequest &,
llvm::function_ref<void(const Ref &)>) const override;
+ bool containedRefs(
+ const ContainedRefsRequest &,
+ llvm::function_ref<void(const ContainedRefsResult &)>) const override;
void relations(const RelationsRequest &,
llvm::function_ref<void(const SymbolID &, const Symbol &)>)
const override;
diff --git a/clang-tools-extra/clangd/index/ProjectAware.cpp b/clang-tools-extra/clangd/index/ProjectAware.cpp
index 2c6f8273b35d0e..9836f0130362a0 100644
--- a/clang-tools-extra/clangd/index/ProjectAware.cpp
+++ b/clang-tools-extra/clangd/index/ProjectAware.cpp
@@ -35,6 +35,10 @@ class ProjectAwareIndex : public SymbolIndex {
/// Query all indexes while prioritizing the associated one (if any).
bool refs(const RefsRequest &Req,
llvm::function_ref<void(const Ref &)> Callback) const override;
+ /// Query all indexes while prioritizing the associated one (if any).
+ bool containedRefs(const ContainedRefsRequest &Req,
+ llvm::function_ref<void(const ContainedRefsResult &)>
+ Callback) const override;
/// Queries only the associates index when Req.RestrictForCodeCompletion is
/// set, otherwise queries all.
@@ -94,6 +98,15 @@ bool ProjectAwareIndex::refs(
return false;
}
+bool ProjectAwareIndex::containedRefs(
+ const ContainedRefsRequest &Req,
+ llvm::function_ref<void(const ContainedRefsResult &)> Callback) const {
+ trace::Span Tracer("ProjectAwareIndex::refersTo");
+ if (auto *Idx = getIndex())
+ return Idx->containedRefs(Req, Callback);
+ return false;
+}
+
bool ProjectAwareIndex::fuzzyFind(
const FuzzyFindRequest &Req,
llvm::function_ref<void(const Symbol &)> Callback) const {
diff --git a/clang-tools-extra/clangd/index/Ref.h b/clang-tools-extra/clangd/index/Ref.h
index 6e383e2ade3d25..870f77f56e6cb3 100644
--- a/clang-tools-extra/clangd/index/Ref.h
+++ b/clang-tools-extra/clangd/index/Ref.h
@@ -63,6 +63,9 @@ enum class RefKind : uint8_t {
// ^ this references Foo, but does not explicitly spell out its name
// };
Spelled = 1 << 3,
+ // A reference which is a call. Used as a filter for which references
+ // to store in data structures used for computing outgoing calls.
+ Call = 1 << 4,
All = Declaration | Definition | Reference | Spelled,
};
diff --git a/clang-tools-extra/clangd/index/SymbolCollector.cpp b/clang-tools-extra/clangd/index/SymbolCollector.cpp
index bf838e53f2a21e..a1e9afebab0654 100644
--- a/clang-tools-extra/clangd/index/SymbolCollector.cpp
+++ b/clang-tools-extra/clangd/index/SymbolCollector.cpp
@@ -18,6 +18,7 @@
#include "clang-include-cleaner/Record.h"
#include "clang-include-cleaner/Types.h"
#include "index/CanonicalIncludes.h"
+#include "index/Ref.h"
#include "index/Relation.h"
#include "index/Symbol.h"
#include "index/SymbolID.h"
@@ -612,7 +613,7 @@ bool SymbolCollector::handleDeclOccurrence(
auto FileLoc = SM.getFileLoc(Loc);
auto FID = SM.getFileID(FileLoc);
if (Opts.RefsInHeaders || FID == SM.getMainFileID()) {
- addRef(ID, SymbolRef{FileLoc, FID, Roles,
+ addRef(ID, SymbolRef{FileLoc, FID, Roles, index::getSymbolInfo(ND).Kind,
getRefContainer(ASTNode.Parent, Opts),
isSpelled(FileLoc, *ND)});
}
@@ -722,8 +723,10 @@ bool SymbolCollector::handleMacroOccurrence(const IdentifierInfo *Name,
// FIXME: Populate container information for macro references.
// FIXME: All MacroRefs are marked as Spelled now, but this should be
// checked.
- addRef(ID, SymbolRef{Loc, SM.getFileID(Loc), Roles, /*Container=*/nullptr,
- /*Spelled=*/true});
+ addRef(ID,
+ SymbolRef{Loc, SM.getFileID(Loc), Roles, index::SymbolKind::Macro,
+ /*Container=*/nullptr,
+ /*Spelled=*/true});
}
// Collect symbols.
@@ -1081,6 +1084,14 @@ bool SymbolCollector::shouldIndexFile(FileID FID) {
return I.first->second;
}
+static bool refIsCall(index::SymbolKind Kind) {
+ using SK = index::SymbolKind;
+ return Kind == SK::Function || Kind == SK::InstanceMethod ||
+ Kind == SK::ClassMethod || Kind == SK::StaticMethod ||
+ Kind == SK::Constructor || Kind == SK::Destructor ||
+ Kind == SK::ConversionFunction;
+}
+
...
[truncated]
|
|
(Not sure what happened here, the PR was somehow created while I was still writing the description. Anyways, updating the title now.) |
HighCommander4
changed the title
|
Since Phabricator has been taken down, I'm resubmitting the patch implementing outgoing calls in call hierarchy that was previously posted at https://reviews.llvm.org/D93829. Previous discussion can still be seen at the Phabricator link which now links to a static snapshot of the review. |
|
@sam-mccall review ping :) I would particularly appreciate feedback on whether I should plan to set aside some time to implement the "deep lookup optimization" (from this comment), or whether the 2.5% increase in index memory usage (which I've achieved by implementing everything except the deep lookup optimization from that comment) is acceptable. |
|
@sam-mccall review ping :) |
|
@sam-mccall also pinging to assist @HighCommander4 🙂 |
|
Shopping around for some potential alternative reviewers :) |
|
I was thinking that it's a real pity this branch is so hard to get merged 😅 I think other than the review it would be 100% done? I mean, no big further modifications would be involved? |
In terms of functionality, I'd say the patch is complete. In terms of performance (and specifically memory usage), it's a matter of judgment of what we consider good enough. The current patch increases the memory usage of the index by 2.5% on the example workload of the LLVM codebase. That's not too bad (and a definite improvement over the 8.2% increase for the original version of the patch), but there is a possibility of doing a more involved rework of the index's in-memory representation (discussed under the name "deep lookup optimization" in the Phabricator thread) which would actually result in a net decrease in memory usage even with the feature. One of the key pieces of feedback I'm hoping to get on this patch is whether the 2.5% memory usage increase is acceptable, or if we should consider the deep lookup optimization as a prerequisite. |
|
@kadircet perhaps you might be able to pick up this review? Or, in the absence of a full review, your opinion on the directional question in this comment would be appreciated as well:
|
|
Hopefully we're closer and closer to a merge, this really is a missing feature. I think this is the only LSP provider supporting just only one of the callHierarchy methods. |
|
@ckandeler, perhaps you would be willing to review this patch? I don't seem to have gotten any traction from my usual reviewers over the course of the past year. |
ckandeler
reviewed
Nov 20, 2024
i-garrison
reviewed
Nov 20, 2024
| @@ -1751,6 +1752,7 @@ static std::optional<HierarchyItem> symbolToHierarchyItem(const Symbol &S, | |||
| } | |||
| HierarchyItem HI; | |||
| HI.name = std::string(S.Name); | |||
| HI.detail = (S.Scope + S.Name).str(); | |||
There was a problem hiding this comment.
adding signature looks a bit more helpful, maybe one day add return type info too
Suggested change
| HI.detail = (S.Scope + S.Name).str(); | |
| HI.detail = (S.Scope + S.Name + S.Signature).str(); |
There was a problem hiding this comment.
I agree, showing the signature here would be quite useful.
I actually started implementing this for incoming calls in clangd/clangd#915, but ran into a complication (our current indexing process doesn't actually store the signature for all functions).
This is solvable with some indexer changes, but I'll defer to a fix for that issue, which will now (once this patch is merged) benefit both incoming and outgoing calls.
i-garrison
reviewed
Nov 20, 2024
| @@ -1700,6 +1700,7 @@ declToHierarchyItem(const NamedDecl &ND, llvm::StringRef TUPath) { | |||
|
|
|||
| HierarchyItem HI; | |||
| HI.name = printName(Ctx, ND); | |||
| // FIXME: Populate HI.detail the way we do in symbolToHierarchyItem? | |||
There was a problem hiding this comment.
is this the place which covers c++ constructors?
There was a problem hiding this comment.
The difference between declToHierarchyItem and symbolToHierarchyItem is the former is used for symbols from the AST, and the latter for symbols from the index. They should both ideally produce the same result, but the implementation (and sometimes the amount of detail available in the respective inputs) is different.
There was a problem hiding this comment.
I see. Ideally there should be some recommended use model for c++ which call hierarchy clients/visualisers (e.g. lsp4e) could implement to show expected function signatures. Like "obtain details field from call hierarchy item and use it as function signature" and then lsp4e could append details to name. We probably can later tune xrefs.cpp implementation to provide required details where needed.
Note that function return value is still not available, and as far as I can see there is no place in protocol to put it in. I would envision language-specific details map for that.
There was a problem hiding this comment.
See also clangd/clangd#1940 for some additional discussion of this topic.
The implementation is very close the the incoming calls implementation. The results of the outgoing calls are expected to be the exact symmetry of the incoming calls. Differential Revision: https://reviews.llvm.org/D93829
HighCommander4
force-pushed
the
outgoing-calls
branch
from
11b9065 to
4b4bbbc
Compare
|
Rebased. |
|
Thanks @ckandeler for the review! I addressed the comments in bad4f72. |
ckandeler
approved these changes
Nov 25, 2024
|
First of all, I want to express my gratitude to all contributors working on the integration of changes and tests. In today's development landscape, especially when working with AI-assisted coding, providing the best possible code context is crucial. The integration of outgoing calls functionality can significantly improve this aspect. The main benefit of implementing outgoing calls would be context extension. This feature would:
When modifying a function, developers would have immediate visibility of all called functions that might need corresponding updates, reducing the risk of introducing bugs. |
Thanks! I'll go ahead and merge. |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/190/builds/10189 Here is the relevant piece of the build log for the reference |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/65/builds/8369 Here is the relevant piece of the build log for the reference |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/134/builds/9267 Here is the relevant piece of the build log for the reference |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/27/builds/2558 Here is the relevant piece of the build log for the reference |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/154/builds/8052 Here is the relevant piece of the build log for the reference |
HighCommander4
added a commit
that referenced
this pull request
Nov 26, 2024
HighCommander4
added a commit
to HighCommander4/llvm-project
that referenced
this pull request
Nov 26, 2024
This reverts commit ca184cf.
|
Reverted in #117668 due to build breakage. |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/95/builds/6644 Here is the relevant piece of the build log for the reference |
The buildkite runs in this PR were green, but the PR was based on an older baseline, and some of the affected files have since changed in ways that require an update to the patch. |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/76/builds/4812 Here is the relevant piece of the build log for the reference |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/164/builds/4934 Here is the relevant piece of the build log for the reference |
|
Resubmitted with the build failures fixed at #117673. |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/56/builds/13090 Here is the relevant piece of the build log for the reference |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/153/builds/15804 Here is the relevant piece of the build log for the reference |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
No description provided.