[clang-tools-extra] [clangd] Re-land "support outgoing calls in call hierarchy" (PR #117673)
HighCommander4 wrote: > Does anyone know the estimated release date for version 20? Based on https://llvm.org/docs/HowToReleaseLLVM.html#annual-release-schedule, approximately March 6. https://github.com/llvm/llvm-project/pull/117673 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] [clangd] Re-land "support outgoing calls in call hierarchy" (PR #117673)
pidgeon777 wrote: > > While I don't have direct experience with LLVM releases, I suppose the > > Outgoing Calls feature will be part of version 20? > > Yep Does anyone know the estimated release date for version 20? https://github.com/llvm/llvm-project/pull/117673 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] [clangd] Re-land "support outgoing calls in call hierarchy" (PR #117673)
HighCommander4 wrote: > > @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](https://reviews.llvm.org/D93829#4654786) would > > be appreciated as well: > > > how would you feel about proceeding with the patch in its current state, > > > with the memory usage increase brought down from 8.2% to 2.5% thanks to > > > the combination of the simple lookup optimization + RefKind filtering, > > > and leaving the "deep lookup optimization" to be explored in a future > > > change? > > I'd definitely prefer the one we discussed in the original review, but I > don't think perfect needs to be enemy of the good, we can surely optimize > data structures here going forward if needed. I filed https://github.com/clangd/clangd/issues/2264 as a follow-up to track implementation of the "deep lookup optimization". https://github.com/llvm/llvm-project/pull/117673 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] [clangd] Re-land "support outgoing calls in call hierarchy" (PR #117673)
HighCommander4 wrote: > While I don't have direct experience with LLVM releases, I suppose the > Outgoing Calls feature will be part of version 20? Yep https://github.com/llvm/llvm-project/pull/117673 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] [clangd] Re-land "support outgoing calls in call hierarchy" (PR #117673)
pidgeon777 wrote: While I don't have direct experience with LLVM releases, I suppose the Outgoing Calls feature will be part of version 20? https://github.com/llvm/llvm-project/pull/117673 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] [clangd] Re-land "support outgoing calls in call hierarchy" (PR #117673)
HighCommander4 wrote: > Any chance this is backportable to the 19.x series? Or is it too dependent on > changes since then? > > Or even better 18.x? (since a lot of downstream tools are dependent on 18.x > and will be a while before they bump to 19) If you're asking about the change being backported to appear in an official 19.x release, then I believe it would not be eligible for that; the release branch only accepts backports of regression and other serious bug fixes, not new features. If you're asking whether the patch would apply cleanly to the 19.x release branch, for the purpose of applying it to some downstream repository that's based on llvm 19.x: while I haven't tried, I expect it should be possible to rebase the patch with fairly minimal changes. This is probably true of 18.x as well. If you try this and have a question about a specific conflict, I'm happy to provide suggestions. https://github.com/llvm/llvm-project/pull/117673 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] [clangd] Re-land "support outgoing calls in call hierarchy" (PR #117673)
catskul wrote: Any chance this is backportable to the 19.x series? Or is it too dependent on changes since then? https://github.com/llvm/llvm-project/pull/117673 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] [clangd] Re-land "support outgoing calls in call hierarchy" (PR #117673)
pidgeon777 wrote: > > I would appreciate confirmation that this will indeed be possible with the > > upcoming **clangd** changes. > > Yes, the returned data is sufficient for clients to implement either of the > above presentations. This is great news, thank you! https://github.com/llvm/llvm-project/pull/117673 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] [clangd] Re-land "support outgoing calls in call hierarchy" (PR #117673)
HighCommander4 wrote: > I would appreciate confirmation that this will indeed be possible with the > upcoming **clangd** changes. Yes, the returned data is sufficient for clients to implement either of the above presentations. https://github.com/llvm/llvm-project/pull/117673 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] [clangd] Re-land "support outgoing calls in call hierarchy" (PR #117673)
pidgeon777 wrote:
Hello everyone, I'm seeking clarification regarding the implementation of
Outgoing Calls support.
The LSP protocol currently allows to implement two display modes for
visualizing outgoing function calls:
```c
#include
void function1(void) {
printf("Executing function 1\n");
}
void function2(void) {
printf("Executing function 2\n");
}
void function3(void) {
printf("Executing function 3\n");
}
int main(void) {
printf("Starting main function\n");
function1();
function2();
function3();
return 0;
}
```
For example, when querying all functions called from `main`, results could be
presented in two ways:
1. Display all the called function occurrences in `main`:
- function1()
- function2()
- function2()
- function3()
2. Display the definitions of the functions called in `main`:
- function1()
- function2()
- function3()
According to one of the developers, this feature should provide all the
necessary data to allow the LSP client to determine the presentation method
(approach 1 or 2). I previously asked about this in another thread (which I
cannot locate now), and I would appreciate confirmation that this will indeed
be possible with the upcoming **clangd** changes.
https://github.com/llvm/llvm-project/pull/117673
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] [clangd] Re-land "support outgoing calls in call hierarchy" (PR #117673)
HighCommander4 wrote: > relanded with benchmark fix in > [61fe67a](https://github.com/llvm/llvm-project/commit/61fe67a4017375fd675f75652e857e837f77fa51) > > changed required -> optional in > [c7ef0ac](https://github.com/llvm/llvm-project/commit/c7ef0ac9fd28cb55b8c7c91a890b365cc688f9a9) Thanks! https://github.com/llvm/llvm-project/pull/117673 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] [clangd] Re-land "support outgoing calls in call hierarchy" (PR #117673)
kadircet wrote: relanded with benchmark fix in 61fe67a4017375fd675f75652e857e837f77fa51 changed required -> optional in c7ef0ac9fd28cb55b8c7c91a890b365cc688f9a9 https://github.com/llvm/llvm-project/pull/117673 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] [clangd] Re-land "support outgoing calls in call hierarchy" (PR #117673)
https://github.com/HighCommander4 closed https://github.com/llvm/llvm-project/pull/117673 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] [clangd] Re-land "support outgoing calls in call hierarchy" (PR #117673)
HighCommander4 wrote: Updated to remove the default arguments. Thanks for the reviews! https://github.com/llvm/llvm-project/pull/117673 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] [clangd] Re-land "support outgoing calls in call hierarchy" (PR #117673)
@@ -163,7 +163,7 @@ class Checker {
unsigned ErrCount = 0;
Checker(llvm::StringRef File, const ClangdLSPServer::Options &Opts)
- : File(File), Opts(Opts) {}
+ : File(File), Opts(Opts), Index(/*SupportContainedRefs=*/false) {}
HighCommander4 wrote:
My original thinking was that `--check` does not perform any outgoing calls
requests. But thinking more about it, a common use of `--check` is to reproduce
crashes, so the more codepaths it executes (including the one behind this flag)
the better in that regard.
https://github.com/llvm/llvm-project/pull/117673
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] [clangd] Re-land "support outgoing calls in call hierarchy" (PR #117673)
@@ -108,7 +109,7 @@ class FileSymbols {
/// FIXME: Expose an interface to remove files that are closed.
class FileIndex : public MergedIndex {
public:
- FileIndex();
+ FileIndex(bool SupportContainedRefs = true);
kadircet wrote:
same with the default param
https://github.com/llvm/llvm-project/pull/117673
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] [clangd] Re-land "support outgoing calls in call hierarchy" (PR #117673)
@@ -69,7 +69,7 @@ enum class DuplicateHandling {
/// locking when we swap or obtain references to snapshots.
class FileSymbols {
public:
- FileSymbols(IndexContents IdxContents);
+ FileSymbols(IndexContents IdxContents, bool SupportContainedRefs = true);
kadircet wrote:
can we avoid the default param here?
https://github.com/llvm/llvm-project/pull/117673
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] [clangd] Re-land "support outgoing calls in call hierarchy" (PR #117673)
@@ -163,7 +163,7 @@ class Checker {
unsigned ErrCount = 0;
Checker(llvm::StringRef File, const ClangdLSPServer::Options &Opts)
- : File(File), Opts(Opts) {}
+ : File(File), Opts(Opts), Index(/*SupportContainedRefs=*/false) {}
kadircet wrote:
why false here? i think it's better if we use whatever clangd-server's default
is
https://github.com/llvm/llvm-project/pull/117673
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] [clangd] Re-land "support outgoing calls in call hierarchy" (PR #117673)
https://github.com/kadircet approved this pull request. thanks, mostly LG! i think not leaving the default parameter values are important here to make sure we don't forget to pass the top-level config in any of the relevant places (both now, but also in the future). i know it probably comes with a bunch of changes to tests though :/ (i wish clangd was better at updating signatures in such cases!) https://github.com/llvm/llvm-project/pull/117673 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] [clangd] Re-land "support outgoing calls in call hierarchy" (PR #117673)
@@ -36,7 +36,8 @@ class Dex : public SymbolIndex {
public:
// All data must outlive this index.
template
- Dex(SymbolRange &&Symbols, RefsRange &&Refs, RelationsRange &&Relations)
+ Dex(SymbolRange &&Symbols, RefsRange &&Refs, RelationsRange &&Relations,
+ bool SupportContainedRefs = true)
kadircet wrote:
again the default
https://github.com/llvm/llvm-project/pull/117673
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] [clangd] Re-land "support outgoing calls in call hierarchy" (PR #117673)
@@ -64,16 +65,18 @@ class Dex : public SymbolIndex {
typename FileRange, typename Payload>
Dex(SymbolRange &&Symbols, RefsRange &&Refs, RelationsRange &&Relations,
FileRange &&Files, IndexContents IdxContents, Payload &&BackingData,
- size_t BackingDataSize)
+ size_t BackingDataSize, bool SupportContainedRefs = true)
: Dex(std::forward(Symbols), std::forward(Refs),
std::forward(Relations),
-std::forward(BackingData), BackingDataSize) {
+std::forward(BackingData), BackingDataSize,
+SupportContainedRefs) {
this->Files = std::forward(Files);
this->IdxContents = IdxContents;
}
/// Builds an index from slabs. The index takes ownership of the slab.
- static std::unique_ptr build(SymbolSlab, RefSlab, RelationSlab);
+ static std::unique_ptr build(SymbolSlab, RefSlab, RelationSlab,
+bool SupportContainedRefs = true);
kadircet wrote:
and here
https://github.com/llvm/llvm-project/pull/117673
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] [clangd] Re-land "support outgoing calls in call hierarchy" (PR #117673)
@@ -83,7 +83,8 @@ std::string toYAML(const Ref &); // Build an in-memory static index from an index file. // The size should be relatively small, so data can be managed in memory. std::unique_ptr loadIndex(llvm::StringRef Filename, - SymbolOrigin Origin, bool UseDex = true); + SymbolOrigin Origin, bool UseDex = true, + bool SupportContainedRefs = true); kadircet wrote: avoiding the default here as well https://github.com/llvm/llvm-project/pull/117673 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] [clangd] Re-land "support outgoing calls in call hierarchy" (PR #117673)
https://github.com/kadircet edited https://github.com/llvm/llvm-project/pull/117673 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] [clangd] Re-land "support outgoing calls in call hierarchy" (PR #117673)
@@ -88,9 +90,12 @@ struct UpdateIndexCallbacks : public ParsingCallbacks {
indexStdlib(CI, std::move(*Loc));
// FIndex outlives the UpdateIndexCallbacks.
-auto Task = [FIndex(FIndex), Path(Path.str()), Version(Version.str()),
+auto Task = [this, FIndex(FIndex), Path(Path.str()),
Version(Version.str()),
ASTCtx(std::move(ASTCtx)), PI(std::move(PI))]() mutable {
trace::Span Tracer("PreambleIndexing");
+ std::optional WithProvidedContext;
+ if (ContextProvider)
+WithProvidedContext.emplace(ContextProvider(""));
kadircet wrote:
why do we need to caputre contextprovider here, can we just use
`Context::current()`? `onPreambleAST` is only invoked by preamble-thread, which
already has the relevant context set up. you can just capture and set it via
`[Ctx(Context::current().clone()] { WithContext WithCtx(std::move(Ctx)); ... };`
https://github.com/llvm/llvm-project/pull/117673
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] [clangd] Re-land "support outgoing calls in call hierarchy" (PR #117673)
HighCommander4 wrote: > I was trying to imply a knob like > https://github.com/llvm/llvm-project/blob/main/clang-tools-extra/clangd/ClangdServer.h#L111. > I don't think the knob needs to be exposed to end-users directly, only > having programmatic access is fine initially. Thanks, that is indeed quite a bit simpler. I reworked the flag to be just a ClangdServer option, and plumbed it in explicitly, in https://github.com/llvm/llvm-project/pull/117673/commits/0aee7953ef1285bb327f20a01389e554fb44ffa8. https://github.com/llvm/llvm-project/pull/117673 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] [clangd] Re-land "support outgoing calls in call hierarchy" (PR #117673)
@@ -149,15 +150,18 @@ void Dex::buildIndex() {
InvertedIndex = std::move(Builder).build();
// Build RevRefs
- for (const auto &[ID, RefList] : Refs)
-for (const auto &R : RefList)
- if ((R.Kind & ContainedRefsRequest::SupportedRefKinds) !=
- RefKind::Unknown)
-RevRefs.emplace_back(R, ID);
- // Sort by container ID so we can use binary search for lookup.
- llvm::sort(RevRefs, [](const RevRef &A, const RevRef &B) {
-return A.ref().Container < B.ref().Container;
- });
+ if (Config::current().CallHierarchy.OutgoingCalls) {
+vlog("WALDOWALDO Building RevRefs");
HighCommander4 wrote:
Oops, debug statement left in accidentally :laughing:
https://github.com/llvm/llvm-project/pull/117673
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] [clangd] Re-land "support outgoing calls in call hierarchy" (PR #117673)
https://github.com/kadircet edited https://github.com/llvm/llvm-project/pull/117673 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] [clangd] Re-land "support outgoing calls in call hierarchy" (PR #117673)
https://github.com/kadircet requested changes to this pull request. I am sorry for my poor choice of words around `config knob`, I was trying to imply a knob like https://github.com/llvm/llvm-project/blob/main/clang-tools-extra/clangd/ClangdServer.h#L111. I don't think the knob needs to be exposed to end-users directly, only having programmatic access is fine initially. if you choose to expose it right away, i think a command line flag is the most natural way, rather than a config field. I'd rather plumb it explicitly through the interfaces of file-index/file-symbol/dex (and others), i think using `context` here is somewhat abusing the functionality. if you feel like that's too much (especially considering the size of the patch), feel free to leave a FIXME at least, to plumb it properly later on. > Only a user config option is respected, not a project config option. I think > this limitation is necessary, because the index is a global structure rather > than a per-project structure. (Concretely, in the call to the Dex constructor > that happens via BackgroundIndexRebuilder::maybeRebuild(), what path would I > pass to the context provider that gets it to look at an appropriate project > config file?) you're absolutely right here. that's the main reason i don't think config mechanism actually works here. they're mostly meaningful for per-project/file configurable features. not for global ones like index. > Only the Dex logic is conditioned on the option, not the binding of the > outgoingCalls message. This is because the binding of the message happens > fairly early, while we're processing the initialize request. At this point we > haven't loaded config files yet, and if we try, that triggers a > publishDiagnostics message for the config file, which confuses clients who > are expecting an initialize response. again this is resulting from my poor choice of words, if we treat this as part of option struct, we'll have the value at startup time and can decide what to do. https://github.com/llvm/llvm-project/pull/117673 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] [clangd] Re-land "support outgoing calls in call hierarchy" (PR #117673)
@@ -149,15 +150,18 @@ void Dex::buildIndex() {
InvertedIndex = std::move(Builder).build();
// Build RevRefs
- for (const auto &[ID, RefList] : Refs)
-for (const auto &R : RefList)
- if ((R.Kind & ContainedRefsRequest::SupportedRefKinds) !=
- RefKind::Unknown)
-RevRefs.emplace_back(R, ID);
- // Sort by container ID so we can use binary search for lookup.
- llvm::sort(RevRefs, [](const RevRef &A, const RevRef &B) {
-return A.ref().Container < B.ref().Container;
- });
+ if (Config::current().CallHierarchy.OutgoingCalls) {
+vlog("WALDOWALDO Building RevRefs");
kadircet wrote:
i don't think waldo should be this easy to find.
https://github.com/llvm/llvm-project/pull/117673
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] [clangd] Re-land "support outgoing calls in call hierarchy" (PR #117673)
@@ -149,15 +150,18 @@ void Dex::buildIndex() {
InvertedIndex = std::move(Builder).build();
// Build RevRefs
- for (const auto &[ID, RefList] : Refs)
-for (const auto &R : RefList)
- if ((R.Kind & ContainedRefsRequest::SupportedRefKinds) !=
- RefKind::Unknown)
-RevRefs.emplace_back(R, ID);
- // Sort by container ID so we can use binary search for lookup.
- llvm::sort(RevRefs, [](const RevRef &A, const RevRef &B) {
-return A.ref().Container < B.ref().Container;
- });
+ if (Config::current().CallHierarchy.OutgoingCalls) {
kadircet wrote:
nit: early exit
https://github.com/llvm/llvm-project/pull/117673
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] [clangd] Re-land "support outgoing calls in call hierarchy" (PR #117673)
@@ -88,9 +90,12 @@ struct UpdateIndexCallbacks : public ParsingCallbacks {
indexStdlib(CI, std::move(*Loc));
// FIndex outlives the UpdateIndexCallbacks.
-auto Task = [FIndex(FIndex), Path(Path.str()), Version(Version.str()),
+auto Task = [this, FIndex(FIndex), Path(Path.str()),
Version(Version.str()),
ASTCtx(std::move(ASTCtx)), PI(std::move(PI))]() mutable {
trace::Span Tracer("PreambleIndexing");
+ std::optional WithProvidedContext;
+ if (ContextProvider)
+WithProvidedContext.emplace(ContextProvider(""));
HighCommander4 wrote:
@kadircet Not sure if this is important: it disables the building of the
`RevRefs` data structure for the dynamic index.
While we do have a `Path` available to pass to the context provider here, I'm
not passing it to remain consistent with the background index codepath, which
only looks at the user config as explained in my previous comment.
(Also, is it fine to capture `this` in this lambda, lifetime-wise?)
https://github.com/llvm/llvm-project/pull/117673
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] [clangd] Re-land "support outgoing calls in call hierarchy" (PR #117673)
HighCommander4 wrote: @kadircet Thanks for having a look. I made a few updates to the PR: * Implemented remote index support (https://github.com/llvm/llvm-project/pull/117673/commits/d380984cecf18b53b6e2fdd4266702dcb8ea513a) * Implemented a config option (https://github.com/llvm/llvm-project/pull/117673/commits/02bd8f6e342c7dfbedd18c791c9e73e3c0ccb385). I did run into some challenges/limitations here: * Only a user config option is respected, not a project config option. I think this limitation is necessary, because the index is a global structure rather than a per-project structure. (Concretely, in the call to the `Dex` constructor that happens via `BackgroundIndexRebuilder::maybeRebuild()`, what path would I pass to the context provider that gets it to look at an appropriate project config file?) * Only the `Dex` logic is conditioned on the option, not the binding of the `outgoingCalls` message. This is because the binding of the message happens fairly early, while we're processing the `initialize` request. At this point we haven't loaded config files yet, and if we try, that triggers a `publishDiagnostics` message for the config file, which confuses clients who are expecting an `initialize` response. * Bumped index version (https://github.com/llvm/llvm-project/pull/117673/commits/c493674c8519133e1e202194fa55564e75c5a08c) https://github.com/llvm/llvm-project/pull/117673 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] [clangd] Re-land "support outgoing calls in call hierarchy" (PR #117673)
https://github.com/HighCommander4 updated
https://github.com/llvm/llvm-project/pull/117673
>From 65f93cd05d9d792bba9a07bda738503f43221221 Mon Sep 17 00:00:00 2001
From: Quentin Chateau
Date: Mon, 18 Sep 2023 03:01:03 -0400
Subject: [PATCH 1/8] [clangd] Support outgoing calls in call hierarchy
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
---
clang-tools-extra/clangd/ClangdLSPServer.cpp | 7 +
clang-tools-extra/clangd/ClangdLSPServer.h| 3 +
clang-tools-extra/clangd/ClangdServer.cpp | 9 +
clang-tools-extra/clangd/ClangdServer.h | 4 +
clang-tools-extra/clangd/XRefs.cpp| 63
clang-tools-extra/clangd/XRefs.h | 3 +
clang-tools-extra/clangd/index/Index.cpp | 5 +
clang-tools-extra/clangd/index/Index.h| 22 ++
clang-tools-extra/clangd/index/MemIndex.cpp | 19 ++
clang-tools-extra/clangd/index/MemIndex.h | 4 +
clang-tools-extra/clangd/index/Merge.cpp | 34 +++
clang-tools-extra/clangd/index/Merge.h| 3 +
.../clangd/index/ProjectAware.cpp | 13 +
clang-tools-extra/clangd/index/dex/Dex.cpp| 25 ++
clang-tools-extra/clangd/index/dex/Dex.h | 18 ++
.../clangd/test/type-hierarchy-ext.test | 2 +
.../clangd/test/type-hierarchy.test | 51 +---
.../clangd/unittests/CallHierarchyTests.cpp | 277 +-
.../clangd/unittests/CodeCompleteTests.cpp| 6 +
.../clangd/unittests/RenameTests.cpp | 12 +
20 files changed, 457 insertions(+), 123 deletions(-)
diff --git a/clang-tools-extra/clangd/ClangdLSPServer.cpp
b/clang-tools-extra/clangd/ClangdLSPServer.cpp
index 05dd313d0a0d35..1391dcaa0c81a4 100644
--- a/clang-tools-extra/clangd/ClangdLSPServer.cpp
+++ b/clang-tools-extra/clangd/ClangdLSPServer.cpp
@@ -1415,6 +1415,12 @@ void ClangdLSPServer::onInlayHint(const InlayHintsParams
&Params,
std::move(Reply));
}
+void ClangdLSPServer::onCallHierarchyOutgoingCalls(
+const CallHierarchyOutgoingCallsParams &Params,
+Callback> Reply) {
+ Server->outgoingCalls(Params.item, std::move(Reply));
+}
+
void ClangdLSPServer::applyConfiguration(
const ConfigurationSettings &Settings) {
// Per-file update to the compilation database.
@@ -1693,6 +1699,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 0b8e4720f53236..597fd9de7ff688 100644
--- a/clang-tools-extra/clangd/ClangdLSPServer.h
+++ b/clang-tools-extra/clangd/ClangdLSPServer.h
@@ -156,6 +156,9 @@ class ClangdLSPServer : private ClangdServer::Callbacks,
void onCallHierarchyIncomingCalls(
const CallHierarchyIncomingCallsParams &,
Callback>);
+ void onCallHierarchyOutgoingCalls(
+ const CallHierarchyOutgoingCallsParams &,
+ Callback>);
void onClangdInlayHints(const InlayHintsParams &,
Callback);
void onInlayHint(const InlayHintsParams &, Callback>);
diff --git a/clang-tools-extra/clangd/ClangdServer.cpp
b/clang-tools-extra/clangd/ClangdServer.cpp
index 9b38be04e7ddd7..63f83bc36f0c69 100644
--- a/clang-tools-extra/clangd/ClangdServer.cpp
+++ b/clang-tools-extra/clangd/ClangdServer.cpp
@@ -912,6 +912,15 @@ void ClangdServer::inlayHints(PathRef File,
std::optional RestrictRange,
WorkScheduler->runWithAST("InlayHints", File, std::move(Action), Transient);
}
+void ClangdServer::outgoingCalls(
+const CallHierarchyItem &Item,
+Callback> 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 a653cdb56b751b..8b6618cf96ccfc 100644
--- a/clang-tools-extra/clangd/ClangdServer.h
+++ b/clang-tools-extra/clangd/ClangdServer.h
@@ -292,6 +292,10
[clang-tools-extra] [clangd] Re-land "support outgoing calls in call hierarchy" (PR #117673)
pidgeon777 wrote: I can't find the watch button for this pull request, so I'm just commenting: hopefully this will finally be merged 🎉. https://github.com/llvm/llvm-project/pull/117673 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] [clangd] Re-land "support outgoing calls in call hierarchy" (PR #117673)
@@ -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,
kadircet wrote:
FWIW, this isn't a backward incompatible change, but if you want
background-index shards to catch up, we should update version in
`index/Serialization.cpp`.
https://github.com/llvm/llvm-project/pull/117673
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] [clangd] Re-land "support outgoing calls in call hierarchy" (PR #117673)
https://github.com/kadircet requested changes to this pull request. sorry for losing track on the original patch, and thanks a lot for driving this to completion! > @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](https://reviews.llvm.org/D93829#4654786) would be > appreciated as well: > > > how would you feel about proceeding with the patch in its current state, > > with the memory usage increase brought down from 8.2% to 2.5% thanks to the > > combination of the simple lookup optimization + RefKind filtering, and > > leaving the "deep lookup optimization" to be explored in a future change? I'd definitely prefer the one we discussed in the original review, but I don't think perfect needs to be enemy of the good, we can surely optimize data structures here going forward if needed. I am still worried about certain deployments (chromium remote-index service, and some internal ones we have). Since the optimization relies on filtering by kind, memory usage increase might be more severe for projects with different structure. So if it isn't going to be too annoying to ask at this point, can we make all of this a config knob? Both handling of `callHierarchy/outgoingCalls` and population of relevant structures in `DexIndex`. Option can be turned on by default, I'd like to make sure we have a quick way out if some of those deployments start OOM'ing. https://github.com/llvm/llvm-project/pull/117673 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] [clangd] Re-land "support outgoing calls in call hierarchy" (PR #117673)
https://github.com/kadircet edited https://github.com/llvm/llvm-project/pull/117673 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] [clangd] Re-land "support outgoing calls in call hierarchy" (PR #117673)
HighCommander4 wrote: I've fixed the build failure that was reported by the majority of the buildbots (which is just a new test case using a test utility function that the patch renames). However, there is an additional build failured reported by [this buildbot](https://github.com/llvm/llvm-project/pull/77556#issuecomment-2499665944) which only applies to builds with `-DCLANGD_ENABLE_REMOTE=ON`. There is a `SymbolIndex` implementation which is only compiled in that configuration (`clang::clangd::remote::IndexClient`), which needs to be implement the new virtual method `containedRefs()` introduced by the patch. https://github.com/llvm/llvm-project/pull/117673 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] [clangd] Re-land "support outgoing calls in call hierarchy" (PR #117673)
llvmbot wrote:
@llvm/pr-subscribers-clang-tools-extra
Author: Nathan Ridge (HighCommander4)
Changes
Previously reviewed at https://github.com/llvm/llvm-project/pull/77556 but was
backed out due to build failures.
---
Patch is 48.62 KiB, truncated to 20.00 KiB below, full version:
https://github.com/llvm/llvm-project/pull/117673.diff
23 Files Affected:
- (modified) clang-tools-extra/clangd/ClangdLSPServer.cpp (+7)
- (modified) clang-tools-extra/clangd/ClangdLSPServer.h (+3)
- (modified) clang-tools-extra/clangd/ClangdServer.cpp (+9)
- (modified) clang-tools-extra/clangd/ClangdServer.h (+4)
- (modified) clang-tools-extra/clangd/XRefs.cpp (+59)
- (modified) clang-tools-extra/clangd/XRefs.h (+3)
- (modified) clang-tools-extra/clangd/index/Index.cpp (+5)
- (modified) clang-tools-extra/clangd/index/Index.h (+35)
- (modified) clang-tools-extra/clangd/index/MemIndex.cpp (+20)
- (modified) clang-tools-extra/clangd/index/MemIndex.h (+4)
- (modified) clang-tools-extra/clangd/index/Merge.cpp (+34)
- (modified) clang-tools-extra/clangd/index/Merge.h (+3)
- (modified) clang-tools-extra/clangd/index/ProjectAware.cpp (+13)
- (modified) clang-tools-extra/clangd/index/Ref.h (+3)
- (modified) clang-tools-extra/clangd/index/SymbolCollector.cpp (+17-3)
- (modified) clang-tools-extra/clangd/index/SymbolCollector.h (+1)
- (modified) clang-tools-extra/clangd/index/dex/Dex.cpp (+42)
- (modified) clang-tools-extra/clangd/index/dex/Dex.h (+20)
- (modified) clang-tools-extra/clangd/test/type-hierarchy-ext.test (+2)
- (modified) clang-tools-extra/clangd/test/type-hierarchy.test (+2)
- (modified) clang-tools-extra/clangd/unittests/CallHierarchyTests.cpp
(+204-73)
- (modified) clang-tools-extra/clangd/unittests/CodeCompleteTests.cpp (+6)
- (modified) clang-tools-extra/clangd/unittests/RenameTests.cpp (+12)
``diff
diff --git a/clang-tools-extra/clangd/ClangdLSPServer.cpp
b/clang-tools-extra/clangd/ClangdLSPServer.cpp
index 05dd313d0a0d35..1391dcaa0c81a4 100644
--- a/clang-tools-extra/clangd/ClangdLSPServer.cpp
+++ b/clang-tools-extra/clangd/ClangdLSPServer.cpp
@@ -1415,6 +1415,12 @@ void ClangdLSPServer::onInlayHint(const InlayHintsParams
&Params,
std::move(Reply));
}
+void ClangdLSPServer::onCallHierarchyOutgoingCalls(
+const CallHierarchyOutgoingCallsParams &Params,
+Callback> Reply) {
+ Server->outgoingCalls(Params.item, std::move(Reply));
+}
+
void ClangdLSPServer::applyConfiguration(
const ConfigurationSettings &Settings) {
// Per-file update to the compilation database.
@@ -1693,6 +1699,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 0b8e4720f53236..597fd9de7ff688 100644
--- a/clang-tools-extra/clangd/ClangdLSPServer.h
+++ b/clang-tools-extra/clangd/ClangdLSPServer.h
@@ -156,6 +156,9 @@ class ClangdLSPServer : private ClangdServer::Callbacks,
void onCallHierarchyIncomingCalls(
const CallHierarchyIncomingCallsParams &,
Callback>);
+ void onCallHierarchyOutgoingCalls(
+ const CallHierarchyOutgoingCallsParams &,
+ Callback>);
void onClangdInlayHints(const InlayHintsParams &,
Callback);
void onInlayHint(const InlayHintsParams &, Callback>);
diff --git a/clang-tools-extra/clangd/ClangdServer.cpp
b/clang-tools-extra/clangd/ClangdServer.cpp
index 9b38be04e7ddd7..63f83bc36f0c69 100644
--- a/clang-tools-extra/clangd/ClangdServer.cpp
+++ b/clang-tools-extra/clangd/ClangdServer.cpp
@@ -912,6 +912,15 @@ void ClangdServer::inlayHints(PathRef File,
std::optional RestrictRange,
WorkScheduler->runWithAST("InlayHints", File, std::move(Action), Transient);
}
+void ClangdServer::outgoingCalls(
+const CallHierarchyItem &Item,
+Callback> 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
i
[clang-tools-extra] [clangd] Re-land "support outgoing calls in call hierarchy" (PR #117673)
https://github.com/HighCommander4 created
https://github.com/llvm/llvm-project/pull/117673
Previously reviewed at https://github.com/llvm/llvm-project/pull/77556 but was
backed out due to build failures.
>From 65f93cd05d9d792bba9a07bda738503f43221221 Mon Sep 17 00:00:00 2001
From: Quentin Chateau
Date: Mon, 18 Sep 2023 03:01:03 -0400
Subject: [PATCH 1/5] [clangd] Support outgoing calls in call hierarchy
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
---
clang-tools-extra/clangd/ClangdLSPServer.cpp | 7 +
clang-tools-extra/clangd/ClangdLSPServer.h| 3 +
clang-tools-extra/clangd/ClangdServer.cpp | 9 +
clang-tools-extra/clangd/ClangdServer.h | 4 +
clang-tools-extra/clangd/XRefs.cpp| 63
clang-tools-extra/clangd/XRefs.h | 3 +
clang-tools-extra/clangd/index/Index.cpp | 5 +
clang-tools-extra/clangd/index/Index.h| 22 ++
clang-tools-extra/clangd/index/MemIndex.cpp | 19 ++
clang-tools-extra/clangd/index/MemIndex.h | 4 +
clang-tools-extra/clangd/index/Merge.cpp | 34 +++
clang-tools-extra/clangd/index/Merge.h| 3 +
.../clangd/index/ProjectAware.cpp | 13 +
clang-tools-extra/clangd/index/dex/Dex.cpp| 25 ++
clang-tools-extra/clangd/index/dex/Dex.h | 18 ++
.../clangd/test/type-hierarchy-ext.test | 2 +
.../clangd/test/type-hierarchy.test | 51 +---
.../clangd/unittests/CallHierarchyTests.cpp | 277 +-
.../clangd/unittests/CodeCompleteTests.cpp| 6 +
.../clangd/unittests/RenameTests.cpp | 12 +
20 files changed, 457 insertions(+), 123 deletions(-)
diff --git a/clang-tools-extra/clangd/ClangdLSPServer.cpp
b/clang-tools-extra/clangd/ClangdLSPServer.cpp
index 05dd313d0a0d35..1391dcaa0c81a4 100644
--- a/clang-tools-extra/clangd/ClangdLSPServer.cpp
+++ b/clang-tools-extra/clangd/ClangdLSPServer.cpp
@@ -1415,6 +1415,12 @@ void ClangdLSPServer::onInlayHint(const InlayHintsParams
&Params,
std::move(Reply));
}
+void ClangdLSPServer::onCallHierarchyOutgoingCalls(
+const CallHierarchyOutgoingCallsParams &Params,
+Callback> Reply) {
+ Server->outgoingCalls(Params.item, std::move(Reply));
+}
+
void ClangdLSPServer::applyConfiguration(
const ConfigurationSettings &Settings) {
// Per-file update to the compilation database.
@@ -1693,6 +1699,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 0b8e4720f53236..597fd9de7ff688 100644
--- a/clang-tools-extra/clangd/ClangdLSPServer.h
+++ b/clang-tools-extra/clangd/ClangdLSPServer.h
@@ -156,6 +156,9 @@ class ClangdLSPServer : private ClangdServer::Callbacks,
void onCallHierarchyIncomingCalls(
const CallHierarchyIncomingCallsParams &,
Callback>);
+ void onCallHierarchyOutgoingCalls(
+ const CallHierarchyOutgoingCallsParams &,
+ Callback>);
void onClangdInlayHints(const InlayHintsParams &,
Callback);
void onInlayHint(const InlayHintsParams &, Callback>);
diff --git a/clang-tools-extra/clangd/ClangdServer.cpp
b/clang-tools-extra/clangd/ClangdServer.cpp
index 9b38be04e7ddd7..63f83bc36f0c69 100644
--- a/clang-tools-extra/clangd/ClangdServer.cpp
+++ b/clang-tools-extra/clangd/ClangdServer.cpp
@@ -912,6 +912,15 @@ void ClangdServer::inlayHints(PathRef File,
std::optional RestrictRange,
WorkScheduler->runWithAST("InlayHints", File, std::move(Action), Transient);
}
+void ClangdServer::outgoingCalls(
+const CallHierarchyItem &Item,
+Callback> 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 a653cdb56b751b..8b6618cf96ccfc 10
