[clang-tools-extra] [clangd] Store full decl/def range with symbol locations (PR #118102)
damnskippy wrote: Been following this thread with much interest albeit selfishly a bit since I was of the impression this PR will pave the way for supporting LocationLink. [Ref: clangd/clangd/discussions/2274] https://github.com/llvm/llvm-project/pull/118102 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] [clangd] Store full decl/def range with symbol locations (PR #118102)
kadircet wrote: > Could you say more about this? What happened to ClangdModules? Sorry, apparently I even forgot its proper name, i meant `clangd::FeatureModule`. Nothing bad happened, we built this infra but didn't get a chance to polish and use it afterwards. We wanted to migrate any non-core components into this bit slowly (clang-tidy, include-cleaner, even LSP features themselves) to both make clangd features more plug&play but also ensure less maintenance burden in the core infra. https://github.com/llvm/llvm-project/pull/118102 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] [clangd] Store full decl/def range with symbol locations (PR #118102)
HighCommander4 wrote: > (For example `ClangdModules` was an action we were trying to take in this > direction. It actually aims to provide people infrastructure to implement > their desired functionality in clangd, while limiting resource and > maintenance costs to only that specific feature and its users. but then we > took an arrow in the knee) Could you say more about this? What happened to `ClangdModules`? https://github.com/llvm/llvm-project/pull/118102 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] [clangd] Store full decl/def range with symbol locations (PR #118102)
HighCommander4 wrote: > I notice that there are four fields in the Symbol struct that are only > relevant for symbols indexed for completion. Presumably these would be > candidates for such an extension block? I think that depends on what proportion of symbols are indexed for completion. I was imagining a fairly simple, fixed-size extension block; that means the overhead of the extension block is incurred for any symbol which needs any of the fields in the extension block. If the completion-related fields are in the extension blocj, and if the proportion of symbols which need them is high, we are then storing larger extension blocks for many symbols, potentially negating the savings we get from having them in the first place. We could also consider more involved extension block designs which are dynamically sized to contain only the fields they need, at the expense of more complexity. https://github.com/llvm/llvm-project/pull/118102 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] [clangd] Store full decl/def range with symbol locations (PR #118102)
kadircet wrote: > We could cut the memory usage overhead of the patch in half by only having it > in one of them. (@kadircet would that change your analysis of the tradeoffs > at all?) I think that'd still be too much considering the functionality we'll get in the end. Others might not share my values here, but my guiding principle has been: We shouldn't increase costs of overall clangd infrastructure, if it isn't going to benefit ~all users and functionality. https://github.com/llvm/llvm-project/pull/127359 is a good example of this. That patch cost us some memory (1%), but in return we got improvements on workflows that are ~always used, and available in pretty much all the editors (signature help & diagnostics). The justification I have for eating costs in such features is, even if we handled them in a narrower focus, we'd still pay those costs ~always (since they're going to be used frequently by all users). Hence if the feature is worth implementing, resource costs are fine to move into core bits of clangd (it would usually result in better complexity/resource usage overall). > A more general musing: it would be nice to have a mechanism for storing > additional information for a subset of symbols, without incurring the > overhead of increasing the representation size of every symbol. As a result of that principle, I am definitely in favor of such mechanisms, bearing maintenance/feature trade-offs in mind. (For example `ClangdModules` was an action we were trying to take in this direction. It actually aims to provide people infrastructure to implement their desired functionality in clangd, while limiting resource and maintenance costs to only that specific feature and its users. but then we took an arrow in the knee) https://github.com/llvm/llvm-project/pull/118102 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] [clangd] Store full decl/def range with symbol locations (PR #118102)
ckandeler wrote: > 2. A more general musing: it would be nice to have a mechanism for storing > additional information for a subset of symbols, without incurring the > overhead of increasing the representation size of **every** symbol. If we had > such a mechanism, we could cut down the overhead of this patch further, by > e.g. only storing the decl/def range for functions rather than all symbol > types. > >* Perhaps this could take the form of a pointer field in `Symbol` which > for a subset of symbols points to additional data stored in the symbol slab? > That would, in and of itself, not reduce the memory footprint compared to > (1), since the pointer would take up 8 bytes, the same as a decl/def range > stored inline. However, we've had use cases like this come up before. (An > example that comes to mind is storing all definitions for symbols with > multiple definitions; see [this > comment](https://github.com/clangd/clangd/issues/1673#issuecomment-1594211998) > and the next few.) So maybe it would be forward-looking to pay the cost for > that pointer once, and unlock the ability to store various extra data in the > future? I notice that there are four fields in the Symbol struct that are only relevant for symbols indexed for completion. Presumably these would be candidates for such an extension block? https://github.com/llvm/llvm-project/pull/118102 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] [clangd] Store full decl/def range with symbol locations (PR #118102)
HighCommander4 wrote: Two quick thoughts, based on reading the discussion so far and a cursory glance at the patch: 1. Do we need to store a decl/def range in both `Symbol::Definition` and `Symbol::CanonicalDeclaration`? We could cut the memory usage overhead of the patch in half by only having it in one of them. (@kadircet would that change your analysis of the tradeoffs at all?) If we only need one decl/def range per symbol, but sometimes it's at the definition and sometimes at the declaration, we could have a top-level field in `Symbol` store it. 2. A more general musing: it would be nice to have a mechanism for storing additional information for a subset of symbols, without incurring the overhead of increasing the representation size of **every** symbol. If we had such a mechanism, we could cut down the overhead of this patch further, by e.g. only storing the decl/def range for functions rather than all symbol types. * Perhaps this could take the form of a pointer field in `Symbol` which for a subset of symbols points to additional data stored in the symbol slab? That would, in and of itself, not reduce the memory footprint compared to (1), since the pointer would take up 8 bytes, the same as a decl/def range stored inline. However, we've had use cases like this come up before. (An example that comes to mind is storing all definitions for symbols with multiple definitions; see [this comment](https://github.com/clangd/clangd/issues/1673#issuecomment-1594211998) and the next few.) So maybe it would be forward-looking to pay the cost for that pointer once, and unlock the ability to store various extra data in the future? https://github.com/llvm/llvm-project/pull/118102 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] [clangd] Store full decl/def range with symbol locations (PR #118102)
ckandeler wrote: *sigh* I want to use clangd for this purpose because it produces correct results particularly with non-trivial constructs, so I'm afraid that using such textual heuristics would kill that advantage. I'll give it a try, though. https://github.com/llvm/llvm-project/pull/118102 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] [clangd] Store full decl/def range with symbol locations (PR #118102)
kadircet wrote:
> My main motivation is to get DefineOutline
I think if we're solely aiming for improvements to DefineOutline, I'd say the
resource trade-off here isn't worth it.
> I don't see how bracket matching can help us get from the name of the
> function to the actual start of the declaration.
so let's say we've got the location for `foo::bar` from the index, as the
declaration to insert after:
```cpp
...
void foo::^bar() {}
...
```
WDYT about an approach that just looks for first trailing `{}` (at the same
level of indentation, i.e. ignoring anything in the function prototype) ? this
might surely have false positives in cases with returning auto-types/concepts
etc. but I think it's still a big enough increment without incurring any costs.
https://github.com/llvm/llvm-project/pull/118102
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] [clangd] Store full decl/def range with symbol locations (PR #118102)
ckandeler wrote: My main motivation is to get DefineOutline out of the proof-of-concept stage where it just dumps the definition somewhere in the same namespace. Usually, the location that best fits the user's expectation is next to the definition of an adjacent declaration. If that adjacent declaration follows the one to be moved, then we want to insert the new definition in front of the existing one, which requires us to know where it starts. I don't see how bracket matching can help us get from the name of the function to the actual start of the declaration. https://github.com/llvm/llvm-project/pull/118102 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] [clangd] Store full decl/def range with symbol locations (PR #118102)
kadircet wrote: I am a little hesitant on the memory growth implications, as this won't be a fixed cost we have in static index, but also a significant increase in dynamic-index memory footprint (which grows proportionally with number of touched files in an editing session). We're going to pay some significant costs in a pretty common workflow, and not in a way that we can turn off (or only pay if we're getting some functionality). Hence I'd like to better understand what we're getting in practice (and if we can't get it any other way). so some high-level questions: - is this functionality really worthwhile? - clangd doesn't support `LocationLink`s today already. do we have plans to add capabilities here? - regarding various `.range` fields in responses, including TypeHierarchyItem.range. what does it really enable for editors/lsp clients? - Why not invest into an implementation that just performs bracket matching in a file? - all of this functionality seem to just care about ~nearest enclosing brace range. parsing declarator itself is hard, braces might not be around in some edge cases where they expand from macro bodies, performing IO might be hard. - but depending on the use cases we want, this might actually be a viable alternative. as define-outline already does IO and has a fallback. type-hierarchy is likely to perform ~limited IO. providing a generic `LocationLink` capability might be hard with this approach. https://github.com/llvm/llvm-project/pull/118102 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] [clangd] Store full decl/def range with symbol locations (PR #118102)
HighCommander4 wrote: Sorry I haven't had a chance to look at this so far; I haven't quite gotten to a place where I'm keeping up with review requests. I will try to make time for it over the coming weeks, but I've also added our other clangd maintainers as reviewers, in case they have some time for it sooner. I would particularly appreciate any guidance from @kadircet on the high-level considerations (e.g. thoughts on the tradeoff of increased memory usage vs. the additional functionality we get for it). https://github.com/llvm/llvm-project/pull/118102 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] [clangd] Store full decl/def range with symbol locations (PR #118102)
ckandeler wrote: ping https://github.com/llvm/llvm-project/pull/118102 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] [clangd] Store full decl/def range with symbol locations (PR #118102)
https://github.com/ckandeler updated
https://github.com/llvm/llvm-project/pull/118102
>From c44237813fb3357dee3f7d17048a7178e67a2422 Mon Sep 17 00:00:00 2001
From: Christian Kandeler
Date: Wed, 27 Nov 2024 13:47:32 +0100
Subject: [PATCH] [clangd] Store full decl/def range with symbol locations
Apart from fixing the linked issue, this is also necessary
for supporting LSP's LocationLink feature and for finding
proper insertion locations in the DefineOutline tweak.
Memory consumption of the background index grows by about ~2.5%.
Closes https://github.com/clangd/clangd/issues/59
---
clang-tools-extra/clangd/CodeComplete.cpp | 4 +-
clang-tools-extra/clangd/FindSymbols.cpp | 34 --
clang-tools-extra/clangd/FindSymbols.h| 8 +-
.../clangd/HeaderSourceSwitch.cpp | 4 +-
clang-tools-extra/clangd/IncludeFixer.cpp | 6 +-
clang-tools-extra/clangd/Quality.cpp | 2 +-
clang-tools-extra/clangd/XRefs.cpp| 65 ++-
clang-tools-extra/clangd/index/FileIndex.cpp | 6 +-
clang-tools-extra/clangd/index/Index.h| 2 +-
clang-tools-extra/clangd/index/Merge.cpp | 13 ++-
clang-tools-extra/clangd/index/Ref.h | 11 +-
.../clangd/index/Serialization.cpp| 34 --
clang-tools-extra/clangd/index/StdLib.cpp | 12 +-
clang-tools-extra/clangd/index/Symbol.h | 8 +-
.../clangd/index/SymbolCollector.cpp | 89 +++
.../clangd/index/SymbolCollector.h| 3 +-
.../clangd/index/SymbolLocation.cpp | 11 +-
.../clangd/index/SymbolLocation.h | 103 +++---
.../clangd/index/YAMLSerialization.cpp| 37 +--
clang-tools-extra/clangd/index/dex/Dex.cpp| 4 +-
clang-tools-extra/clangd/refactor/Rename.cpp | 4 +-
.../clangd/test/Inputs/symbols.test.yaml | 17 ++-
.../index-serialization/Inputs/sample.idx | Bin 470 -> 496 bytes
.../clangd/test/type-hierarchy-ext.test | 8 +-
.../clangd/test/type-hierarchy.test | 8 +-
.../clangd/unittests/BackgroundIndexTests.cpp | 4 +-
.../clangd/unittests/CodeCompleteTests.cpp| 18 +--
.../clangd/unittests/DexTests.cpp | 5 +-
.../clangd/unittests/DiagnosticsTests.cpp | 13 ++-
.../clangd/unittests/FileIndexTests.cpp | 14 +--
.../clangd/unittests/IndexTests.cpp | 45
.../clangd/unittests/SerializationTests.cpp | 23 +++-
.../clangd/unittests/SymbolCollectorTests.cpp | 12 +-
33 files changed, 388 insertions(+), 239 deletions(-)
diff --git a/clang-tools-extra/clangd/CodeComplete.cpp
b/clang-tools-extra/clangd/CodeComplete.cpp
index 2c2d5f0b5ac924..04e4aa2d2d1ca9 100644
--- a/clang-tools-extra/clangd/CodeComplete.cpp
+++ b/clang-tools-extra/clangd/CodeComplete.cpp
@@ -418,7 +418,7 @@ struct CodeCompletionBuilder {
auto Inserted = [&](llvm::StringRef Header)
-> llvm::Expected> {
auto ResolvedDeclaring =
- URI::resolve(C.IndexResult->CanonicalDeclaration.FileURI, FileName);
+ URI::resolve(C.IndexResult->CanonicalDeclaration.fileURI(),
FileName);
if (!ResolvedDeclaring)
return ResolvedDeclaring.takeError();
auto ResolvedInserted = toHeaderFile(Header, FileName);
@@ -451,7 +451,7 @@ struct CodeCompletionBuilder {
} else
log("Failed to generate include insertion edits for adding header "
"(FileURI='{0}', IncludeHeader='{1}') into {2}: {3}",
-C.IndexResult->CanonicalDeclaration.FileURI, Inc.Header, FileName,
+C.IndexResult->CanonicalDeclaration.fileURI(), Inc.Header,
FileName,
ToInclude.takeError());
}
// Prefer includes that do not need edits (i.e. already exist).
diff --git a/clang-tools-extra/clangd/FindSymbols.cpp
b/clang-tools-extra/clangd/FindSymbols.cpp
index 84bcbc1f2ddd3f..646cb261309c80 100644
--- a/clang-tools-extra/clangd/FindSymbols.cpp
+++ b/clang-tools-extra/clangd/FindSymbols.cpp
@@ -54,9 +54,19 @@ bool approximateScopeMatch(llvm::StringRef Scope,
llvm::StringRef Query) {
return Query.empty();
}
+Range indexToLSPRange(const SymbolPosition &SrcStart,
+ const SymbolPosition &SrcEnd) {
+ Position Start, End;
+ Start.line = SrcStart.line();
+ Start.character = SrcStart.column();
+ End.line = SrcEnd.line();
+ End.character = SrcEnd.column();
+ return {Start, End};
+}
+
} // namespace
-llvm::Expected indexToLSPLocation(const SymbolLocation &Loc,
+llvm::Expected indexToLSPLocation(const SymbolNameLocation &Loc,
llvm::StringRef TUPath) {
auto Path = URI::resolve(Loc.FileURI, TUPath);
if (!Path)
@@ -64,17 +74,21 @@ llvm::Expected indexToLSPLocation(const
SymbolLocation &Loc,
Path.takeError());
Location L;
L.uri = URIForFile::canonicalize(*Path, TUPath);
- Position Start, End;
- Start.line = Loc.Start.line();
- Start.character = Loc.Start.column();
- End
[clang-tools-extra] [clangd] Store full decl/def range with symbol locations (PR #118102)
https://github.com/ckandeler updated
https://github.com/llvm/llvm-project/pull/118102
>From 188cbf1a7d3e83c0a558550351a373c1c3475d4e Mon Sep 17 00:00:00 2001
From: Christian Kandeler
Date: Wed, 27 Nov 2024 13:47:32 +0100
Subject: [PATCH] [clangd] Store full decl/def range with symbol locations
Apart from fixing the linked issue, this is also necessary
for supporting LSP's LocationLink feature and for finding
proper insertion locations in the DefineOutline tweak.
Memory consumption of the background index grows by about ~2.5%.
Closes https://github.com/clangd/clangd/issues/59
---
clang-tools-extra/clangd/CodeComplete.cpp | 4 +-
clang-tools-extra/clangd/FindSymbols.cpp | 34 --
clang-tools-extra/clangd/FindSymbols.h| 8 +-
.../clangd/HeaderSourceSwitch.cpp | 4 +-
clang-tools-extra/clangd/IncludeFixer.cpp | 6 +-
clang-tools-extra/clangd/Quality.cpp | 2 +-
clang-tools-extra/clangd/XRefs.cpp| 65 ++-
clang-tools-extra/clangd/index/FileIndex.cpp | 6 +-
clang-tools-extra/clangd/index/Merge.cpp | 13 ++-
clang-tools-extra/clangd/index/Ref.h | 11 +-
.../clangd/index/Serialization.cpp| 36 --
clang-tools-extra/clangd/index/StdLib.cpp | 12 +-
clang-tools-extra/clangd/index/Symbol.h | 8 +-
.../clangd/index/SymbolCollector.cpp | 89 +++
.../clangd/index/SymbolCollector.h| 3 +-
.../clangd/index/SymbolLocation.cpp | 11 +-
.../clangd/index/SymbolLocation.h | 103 +++---
.../clangd/index/YAMLSerialization.cpp| 37 +--
clang-tools-extra/clangd/index/dex/Dex.cpp| 4 +-
clang-tools-extra/clangd/refactor/Rename.cpp | 4 +-
.../clangd/test/Inputs/symbols.test.yaml | 17 ++-
.../index-serialization/Inputs/sample.idx | Bin 470 -> 496 bytes
.../clangd/test/type-hierarchy-ext.test | 8 +-
.../clangd/test/type-hierarchy.test | 8 +-
.../clangd/unittests/BackgroundIndexTests.cpp | 4 +-
.../clangd/unittests/CodeCompleteTests.cpp| 18 +--
.../clangd/unittests/DexTests.cpp | 5 +-
.../clangd/unittests/DiagnosticsTests.cpp | 13 ++-
.../clangd/unittests/FileIndexTests.cpp | 14 +--
.../clangd/unittests/IndexTests.cpp | 45
.../clangd/unittests/SerializationTests.cpp | 23 +++-
.../clangd/unittests/SymbolCollectorTests.cpp | 12 +-
32 files changed, 388 insertions(+), 239 deletions(-)
diff --git a/clang-tools-extra/clangd/CodeComplete.cpp
b/clang-tools-extra/clangd/CodeComplete.cpp
index 2c2d5f0b5ac924..04e4aa2d2d1ca9 100644
--- a/clang-tools-extra/clangd/CodeComplete.cpp
+++ b/clang-tools-extra/clangd/CodeComplete.cpp
@@ -418,7 +418,7 @@ struct CodeCompletionBuilder {
auto Inserted = [&](llvm::StringRef Header)
-> llvm::Expected> {
auto ResolvedDeclaring =
- URI::resolve(C.IndexResult->CanonicalDeclaration.FileURI, FileName);
+ URI::resolve(C.IndexResult->CanonicalDeclaration.fileURI(),
FileName);
if (!ResolvedDeclaring)
return ResolvedDeclaring.takeError();
auto ResolvedInserted = toHeaderFile(Header, FileName);
@@ -451,7 +451,7 @@ struct CodeCompletionBuilder {
} else
log("Failed to generate include insertion edits for adding header "
"(FileURI='{0}', IncludeHeader='{1}') into {2}: {3}",
-C.IndexResult->CanonicalDeclaration.FileURI, Inc.Header, FileName,
+C.IndexResult->CanonicalDeclaration.fileURI(), Inc.Header,
FileName,
ToInclude.takeError());
}
// Prefer includes that do not need edits (i.e. already exist).
diff --git a/clang-tools-extra/clangd/FindSymbols.cpp
b/clang-tools-extra/clangd/FindSymbols.cpp
index 84bcbc1f2ddd3f..646cb261309c80 100644
--- a/clang-tools-extra/clangd/FindSymbols.cpp
+++ b/clang-tools-extra/clangd/FindSymbols.cpp
@@ -54,9 +54,19 @@ bool approximateScopeMatch(llvm::StringRef Scope,
llvm::StringRef Query) {
return Query.empty();
}
+Range indexToLSPRange(const SymbolPosition &SrcStart,
+ const SymbolPosition &SrcEnd) {
+ Position Start, End;
+ Start.line = SrcStart.line();
+ Start.character = SrcStart.column();
+ End.line = SrcEnd.line();
+ End.character = SrcEnd.column();
+ return {Start, End};
+}
+
} // namespace
-llvm::Expected indexToLSPLocation(const SymbolLocation &Loc,
+llvm::Expected indexToLSPLocation(const SymbolNameLocation &Loc,
llvm::StringRef TUPath) {
auto Path = URI::resolve(Loc.FileURI, TUPath);
if (!Path)
@@ -64,17 +74,21 @@ llvm::Expected indexToLSPLocation(const
SymbolLocation &Loc,
Path.takeError());
Location L;
L.uri = URIForFile::canonicalize(*Path, TUPath);
- Position Start, End;
- Start.line = Loc.Start.line();
- Start.character = Loc.Start.column();
- End.line = Loc.End.line();
- End.character = Loc.End.colum
[clang-tools-extra] [clangd] Store full decl/def range with symbol locations (PR #118102)
llvmbot wrote:
@llvm/pr-subscribers-clangd
Author: Christian Kandeler (ckandeler)
Changes
Apart from fixing the linked issue, this is also necessary for supporting LSP's
LocationLink feature and for finding proper insertion locations in the
DefineOutline tweak. Memory consumption of the background index grows by about
~2.5%.
Closes https://github.com/clangd/clangd/issues/59
---
Patch is 69.37 KiB, truncated to 20.00 KiB below, full version:
https://github.com/llvm/llvm-project/pull/118102.diff
32 Files Affected:
- (modified) clang-tools-extra/clangd/CodeComplete.cpp (+2-2)
- (modified) clang-tools-extra/clangd/FindSymbols.cpp (+24-10)
- (modified) clang-tools-extra/clangd/FindSymbols.h (+5-3)
- (modified) clang-tools-extra/clangd/HeaderSourceSwitch.cpp (+2-2)
- (modified) clang-tools-extra/clangd/IncludeFixer.cpp (+3-3)
- (modified) clang-tools-extra/clangd/Quality.cpp (+1-1)
- (modified) clang-tools-extra/clangd/XRefs.cpp (+35-30)
- (modified) clang-tools-extra/clangd/index/FileIndex.cpp (+3-3)
- (modified) clang-tools-extra/clangd/index/Merge.cpp (+7-6)
- (modified) clang-tools-extra/clangd/index/Ref.h (+4-7)
- (modified) clang-tools-extra/clangd/index/Serialization.cpp (+28-8)
- (modified) clang-tools-extra/clangd/index/StdLib.cpp (+6-6)
- (modified) clang-tools-extra/clangd/index/Symbol.h (+4-4)
- (modified) clang-tools-extra/clangd/index/SymbolCollector.cpp (+68-21)
- (modified) clang-tools-extra/clangd/index/SymbolCollector.h (+2-1)
- (modified) clang-tools-extra/clangd/index/SymbolLocation.cpp (+6-5)
- (modified) clang-tools-extra/clangd/index/SymbolLocation.h (+62-41)
- (modified) clang-tools-extra/clangd/index/YAMLSerialization.cpp (+25-12)
- (modified) clang-tools-extra/clangd/index/dex/Dex.cpp (+2-2)
- (modified) clang-tools-extra/clangd/refactor/Rename.cpp (+2-2)
- (modified) clang-tools-extra/clangd/test/Inputs/symbols.test.yaml (+12-5)
- (modified)
clang-tools-extra/clangd/test/index-serialization/Inputs/sample.idx ()
- (modified) clang-tools-extra/clangd/test/type-hierarchy-ext.test (+4-4)
- (modified) clang-tools-extra/clangd/test/type-hierarchy.test (+4-4)
- (modified) clang-tools-extra/clangd/unittests/BackgroundIndexTests.cpp (+2-2)
- (modified) clang-tools-extra/clangd/unittests/CodeCompleteTests.cpp (+9-9)
- (modified) clang-tools-extra/clangd/unittests/DexTests.cpp (+3-2)
- (modified) clang-tools-extra/clangd/unittests/DiagnosticsTests.cpp (+7-6)
- (modified) clang-tools-extra/clangd/unittests/FileIndexTests.cpp (+7-7)
- (modified) clang-tools-extra/clangd/unittests/IndexTests.cpp (+23-22)
- (modified) clang-tools-extra/clangd/unittests/SerializationTests.cpp (+19-4)
- (modified) clang-tools-extra/clangd/unittests/SymbolCollectorTests.cpp (+7-5)
``diff
diff --git a/clang-tools-extra/clangd/CodeComplete.cpp
b/clang-tools-extra/clangd/CodeComplete.cpp
index 2c2d5f0b5ac924..04e4aa2d2d1ca9 100644
--- a/clang-tools-extra/clangd/CodeComplete.cpp
+++ b/clang-tools-extra/clangd/CodeComplete.cpp
@@ -418,7 +418,7 @@ struct CodeCompletionBuilder {
auto Inserted = [&](llvm::StringRef Header)
-> llvm::Expected> {
auto ResolvedDeclaring =
- URI::resolve(C.IndexResult->CanonicalDeclaration.FileURI, FileName);
+ URI::resolve(C.IndexResult->CanonicalDeclaration.fileURI(),
FileName);
if (!ResolvedDeclaring)
return ResolvedDeclaring.takeError();
auto ResolvedInserted = toHeaderFile(Header, FileName);
@@ -451,7 +451,7 @@ struct CodeCompletionBuilder {
} else
log("Failed to generate include insertion edits for adding header "
"(FileURI='{0}', IncludeHeader='{1}') into {2}: {3}",
-C.IndexResult->CanonicalDeclaration.FileURI, Inc.Header, FileName,
+C.IndexResult->CanonicalDeclaration.fileURI(), Inc.Header,
FileName,
ToInclude.takeError());
}
// Prefer includes that do not need edits (i.e. already exist).
diff --git a/clang-tools-extra/clangd/FindSymbols.cpp
b/clang-tools-extra/clangd/FindSymbols.cpp
index 84bcbc1f2ddd3f..646cb261309c80 100644
--- a/clang-tools-extra/clangd/FindSymbols.cpp
+++ b/clang-tools-extra/clangd/FindSymbols.cpp
@@ -54,9 +54,19 @@ bool approximateScopeMatch(llvm::StringRef Scope,
llvm::StringRef Query) {
return Query.empty();
}
+Range indexToLSPRange(const SymbolPosition &SrcStart,
+ const SymbolPosition &SrcEnd) {
+ Position Start, End;
+ Start.line = SrcStart.line();
+ Start.character = SrcStart.column();
+ End.line = SrcEnd.line();
+ End.character = SrcEnd.column();
+ return {Start, End};
+}
+
} // namespace
-llvm::Expected indexToLSPLocation(const SymbolLocation &Loc,
+llvm::Expected indexToLSPLocation(const SymbolNameLocation &Loc,
llvm::StringRef TUPath) {
auto Path = URI::resolve(Loc.FileURI, TUPath);
if (!Path)
@@ -64,17 +74,21 @@ llvm::Expected indexToLSPLocation(const
SymbolLocation &Loc
[clang-tools-extra] [clangd] Store full decl/def range with symbol locations (PR #118102)
llvmbot wrote:
@llvm/pr-subscribers-clang-tools-extra
Author: Christian Kandeler (ckandeler)
Changes
Apart from fixing the linked issue, this is also necessary for supporting LSP's
LocationLink feature and for finding proper insertion locations in the
DefineOutline tweak. Memory consumption of the background index grows by about
~2.5%.
Closes https://github.com/clangd/clangd/issues/59
---
Patch is 69.37 KiB, truncated to 20.00 KiB below, full version:
https://github.com/llvm/llvm-project/pull/118102.diff
32 Files Affected:
- (modified) clang-tools-extra/clangd/CodeComplete.cpp (+2-2)
- (modified) clang-tools-extra/clangd/FindSymbols.cpp (+24-10)
- (modified) clang-tools-extra/clangd/FindSymbols.h (+5-3)
- (modified) clang-tools-extra/clangd/HeaderSourceSwitch.cpp (+2-2)
- (modified) clang-tools-extra/clangd/IncludeFixer.cpp (+3-3)
- (modified) clang-tools-extra/clangd/Quality.cpp (+1-1)
- (modified) clang-tools-extra/clangd/XRefs.cpp (+35-30)
- (modified) clang-tools-extra/clangd/index/FileIndex.cpp (+3-3)
- (modified) clang-tools-extra/clangd/index/Merge.cpp (+7-6)
- (modified) clang-tools-extra/clangd/index/Ref.h (+4-7)
- (modified) clang-tools-extra/clangd/index/Serialization.cpp (+28-8)
- (modified) clang-tools-extra/clangd/index/StdLib.cpp (+6-6)
- (modified) clang-tools-extra/clangd/index/Symbol.h (+4-4)
- (modified) clang-tools-extra/clangd/index/SymbolCollector.cpp (+68-21)
- (modified) clang-tools-extra/clangd/index/SymbolCollector.h (+2-1)
- (modified) clang-tools-extra/clangd/index/SymbolLocation.cpp (+6-5)
- (modified) clang-tools-extra/clangd/index/SymbolLocation.h (+62-41)
- (modified) clang-tools-extra/clangd/index/YAMLSerialization.cpp (+25-12)
- (modified) clang-tools-extra/clangd/index/dex/Dex.cpp (+2-2)
- (modified) clang-tools-extra/clangd/refactor/Rename.cpp (+2-2)
- (modified) clang-tools-extra/clangd/test/Inputs/symbols.test.yaml (+12-5)
- (modified)
clang-tools-extra/clangd/test/index-serialization/Inputs/sample.idx ()
- (modified) clang-tools-extra/clangd/test/type-hierarchy-ext.test (+4-4)
- (modified) clang-tools-extra/clangd/test/type-hierarchy.test (+4-4)
- (modified) clang-tools-extra/clangd/unittests/BackgroundIndexTests.cpp (+2-2)
- (modified) clang-tools-extra/clangd/unittests/CodeCompleteTests.cpp (+9-9)
- (modified) clang-tools-extra/clangd/unittests/DexTests.cpp (+3-2)
- (modified) clang-tools-extra/clangd/unittests/DiagnosticsTests.cpp (+7-6)
- (modified) clang-tools-extra/clangd/unittests/FileIndexTests.cpp (+7-7)
- (modified) clang-tools-extra/clangd/unittests/IndexTests.cpp (+23-22)
- (modified) clang-tools-extra/clangd/unittests/SerializationTests.cpp (+19-4)
- (modified) clang-tools-extra/clangd/unittests/SymbolCollectorTests.cpp (+7-5)
``diff
diff --git a/clang-tools-extra/clangd/CodeComplete.cpp
b/clang-tools-extra/clangd/CodeComplete.cpp
index 2c2d5f0b5ac924..04e4aa2d2d1ca9 100644
--- a/clang-tools-extra/clangd/CodeComplete.cpp
+++ b/clang-tools-extra/clangd/CodeComplete.cpp
@@ -418,7 +418,7 @@ struct CodeCompletionBuilder {
auto Inserted = [&](llvm::StringRef Header)
-> llvm::Expected> {
auto ResolvedDeclaring =
- URI::resolve(C.IndexResult->CanonicalDeclaration.FileURI, FileName);
+ URI::resolve(C.IndexResult->CanonicalDeclaration.fileURI(),
FileName);
if (!ResolvedDeclaring)
return ResolvedDeclaring.takeError();
auto ResolvedInserted = toHeaderFile(Header, FileName);
@@ -451,7 +451,7 @@ struct CodeCompletionBuilder {
} else
log("Failed to generate include insertion edits for adding header "
"(FileURI='{0}', IncludeHeader='{1}') into {2}: {3}",
-C.IndexResult->CanonicalDeclaration.FileURI, Inc.Header, FileName,
+C.IndexResult->CanonicalDeclaration.fileURI(), Inc.Header,
FileName,
ToInclude.takeError());
}
// Prefer includes that do not need edits (i.e. already exist).
diff --git a/clang-tools-extra/clangd/FindSymbols.cpp
b/clang-tools-extra/clangd/FindSymbols.cpp
index 84bcbc1f2ddd3f..646cb261309c80 100644
--- a/clang-tools-extra/clangd/FindSymbols.cpp
+++ b/clang-tools-extra/clangd/FindSymbols.cpp
@@ -54,9 +54,19 @@ bool approximateScopeMatch(llvm::StringRef Scope,
llvm::StringRef Query) {
return Query.empty();
}
+Range indexToLSPRange(const SymbolPosition &SrcStart,
+ const SymbolPosition &SrcEnd) {
+ Position Start, End;
+ Start.line = SrcStart.line();
+ Start.character = SrcStart.column();
+ End.line = SrcEnd.line();
+ End.character = SrcEnd.column();
+ return {Start, End};
+}
+
} // namespace
-llvm::Expected indexToLSPLocation(const SymbolLocation &Loc,
+llvm::Expected indexToLSPLocation(const SymbolNameLocation &Loc,
llvm::StringRef TUPath) {
auto Path = URI::resolve(Loc.FileURI, TUPath);
if (!Path)
@@ -64,17 +74,21 @@ llvm::Expected indexToLSPLocation(const
SymbolLo
[clang-tools-extra] [clangd] Store full decl/def range with symbol locations (PR #118102)
https://github.com/ckandeler created
https://github.com/llvm/llvm-project/pull/118102
Apart from fixing the linked issue, this is also necessary for supporting LSP's
LocationLink feature and for finding proper insertion locations in the
DefineOutline tweak. Memory consumption of the background index grows by about
~2.5%.
Closes https://github.com/clangd/clangd/issues/59
>From 99e7189c6f3c19e3aabeee56793f5683251ecb26 Mon Sep 17 00:00:00 2001
From: Christian Kandeler
Date: Wed, 27 Nov 2024 13:47:32 +0100
Subject: [PATCH] [clangd] Store full decl/def range with symbol locations
Apart from fixing the linked issue, this is also necessary
for supporting LSP's LocationLink feature and for finding
proper insertion locations in the DefineOutline tweak.
Memory consumption of the background index grows by about ~2.5%.
Closes https://github.com/clangd/clangd/issues/59
---
clang-tools-extra/clangd/CodeComplete.cpp | 4 +-
clang-tools-extra/clangd/FindSymbols.cpp | 34 --
clang-tools-extra/clangd/FindSymbols.h| 8 +-
.../clangd/HeaderSourceSwitch.cpp | 4 +-
clang-tools-extra/clangd/IncludeFixer.cpp | 6 +-
clang-tools-extra/clangd/Quality.cpp | 2 +-
clang-tools-extra/clangd/XRefs.cpp| 65 ++-
clang-tools-extra/clangd/index/FileIndex.cpp | 6 +-
clang-tools-extra/clangd/index/Merge.cpp | 13 ++-
clang-tools-extra/clangd/index/Ref.h | 11 +-
.../clangd/index/Serialization.cpp| 36 --
clang-tools-extra/clangd/index/StdLib.cpp | 12 +-
clang-tools-extra/clangd/index/Symbol.h | 8 +-
.../clangd/index/SymbolCollector.cpp | 89 +++
.../clangd/index/SymbolCollector.h| 3 +-
.../clangd/index/SymbolLocation.cpp | 11 +-
.../clangd/index/SymbolLocation.h | 103 +++---
.../clangd/index/YAMLSerialization.cpp| 37 +--
clang-tools-extra/clangd/index/dex/Dex.cpp| 4 +-
clang-tools-extra/clangd/refactor/Rename.cpp | 4 +-
.../clangd/test/Inputs/symbols.test.yaml | 17 ++-
.../index-serialization/Inputs/sample.idx | Bin 470 -> 496 bytes
.../clangd/test/type-hierarchy-ext.test | 8 +-
.../clangd/test/type-hierarchy.test | 8 +-
.../clangd/unittests/BackgroundIndexTests.cpp | 4 +-
.../clangd/unittests/CodeCompleteTests.cpp| 18 +--
.../clangd/unittests/DexTests.cpp | 5 +-
.../clangd/unittests/DiagnosticsTests.cpp | 13 ++-
.../clangd/unittests/FileIndexTests.cpp | 14 +--
.../clangd/unittests/IndexTests.cpp | 45
.../clangd/unittests/SerializationTests.cpp | 23 +++-
.../clangd/unittests/SymbolCollectorTests.cpp | 12 +-
32 files changed, 388 insertions(+), 239 deletions(-)
diff --git a/clang-tools-extra/clangd/CodeComplete.cpp
b/clang-tools-extra/clangd/CodeComplete.cpp
index 2c2d5f0b5ac924..04e4aa2d2d1ca9 100644
--- a/clang-tools-extra/clangd/CodeComplete.cpp
+++ b/clang-tools-extra/clangd/CodeComplete.cpp
@@ -418,7 +418,7 @@ struct CodeCompletionBuilder {
auto Inserted = [&](llvm::StringRef Header)
-> llvm::Expected> {
auto ResolvedDeclaring =
- URI::resolve(C.IndexResult->CanonicalDeclaration.FileURI, FileName);
+ URI::resolve(C.IndexResult->CanonicalDeclaration.fileURI(),
FileName);
if (!ResolvedDeclaring)
return ResolvedDeclaring.takeError();
auto ResolvedInserted = toHeaderFile(Header, FileName);
@@ -451,7 +451,7 @@ struct CodeCompletionBuilder {
} else
log("Failed to generate include insertion edits for adding header "
"(FileURI='{0}', IncludeHeader='{1}') into {2}: {3}",
-C.IndexResult->CanonicalDeclaration.FileURI, Inc.Header, FileName,
+C.IndexResult->CanonicalDeclaration.fileURI(), Inc.Header,
FileName,
ToInclude.takeError());
}
// Prefer includes that do not need edits (i.e. already exist).
diff --git a/clang-tools-extra/clangd/FindSymbols.cpp
b/clang-tools-extra/clangd/FindSymbols.cpp
index 84bcbc1f2ddd3f..646cb261309c80 100644
--- a/clang-tools-extra/clangd/FindSymbols.cpp
+++ b/clang-tools-extra/clangd/FindSymbols.cpp
@@ -54,9 +54,19 @@ bool approximateScopeMatch(llvm::StringRef Scope,
llvm::StringRef Query) {
return Query.empty();
}
+Range indexToLSPRange(const SymbolPosition &SrcStart,
+ const SymbolPosition &SrcEnd) {
+ Position Start, End;
+ Start.line = SrcStart.line();
+ Start.character = SrcStart.column();
+ End.line = SrcEnd.line();
+ End.character = SrcEnd.column();
+ return {Start, End};
+}
+
} // namespace
-llvm::Expected indexToLSPLocation(const SymbolLocation &Loc,
+llvm::Expected indexToLSPLocation(const SymbolNameLocation &Loc,
llvm::StringRef TUPath) {
auto Path = URI::resolve(Loc.FileURI, TUPath);
if (!Path)
@@ -64,17 +74,21 @@ llvm::Expected indexToLSPLocation(c
