[PATCH] D54799: [clangd][WIP] textDocument/CursorInfo method
jkorous created this revision.
jkorous added reviewers: sammccall, arphaman, benlangmuir.
Herald added subscribers: cfe-commits, kadircet, dexonsmith, MaskRay, ioeric,
ilya-biryukov.
Hi,
I implemented `textDocument/cursorInfo` method based on consensus in
https://reviews.llvm.org/D54529.
I'd like to ask for early feedback - what's still missing is relevant client
capability.
Couple things that I'd love to hear opinions about are:
- conditional return in `getCursorInfo` - Should we return for example data
with empty `USR`?
- `containerName` of local variables - It's currently empty even if semantic
parent has a name (say it's a function). (Please search for local.cpp in the
test.) Is that what we want?
- For now I used `getSymbolAtPosition()` as it's simpler and fits the context
better. However I assume I could use this optimization from
`tooling::getNamedDeclAt()` (in a separate patch):
https://github.com/llvm-mirror/clang/blob/master/lib/Tooling/Refactoring/Rename/USRFinder.cpp#L82
- One thing I am wondering about is whether we could use (and whether it's a
significant improvement) some early return in `indexTopLevelDecls` (using
`DeclarationAndMacrosFinder`) also for hover and definition use-cases. Is it
correct to assume that at a given `Location` there can be maximum of one
declaration and one definition?
P. S. Alex and Ben have thanksgiving break now so they'll probably add any
feedback next week.
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D54799
Files:
ClangdLSPServer.cpp
ClangdLSPServer.h
ClangdServer.cpp
ClangdServer.h
Protocol.cpp
Protocol.h
XRefs.cpp
XRefs.h
clangd/cursor-info.test
Index: clangd/cursor-info.test
===
--- /dev/null
+++ clangd/cursor-info.test
@@ -0,0 +1,46 @@
+# RUN: clangd -lit-test < %s | FileCheck %s
+{"jsonrpc":"2.0","id":0,"method":"initialize","params":{"processId":123,"rootPath":"clangd","capabilities":{},"trace":"off"}}
+---
+{"jsonrpc":"2.0","method":"textDocument/didOpen","params":{"textDocument":{"uri":"test:///simple.cpp","languageId":"cpp","version":1,"text":"void foo(); int main() { foo(); }\n"}}}
+---
+{"jsonrpc":"2.0","id":1,"method":"textDocument/cursorInfo","params":{"textDocument":{"uri":"test:///simple.cpp"},"position":{"line":0,"character":27}}}
+# CHECK:"containerName": "",
+# CHECK-NEXT:"id": "CA2EBE44A1D76D2A1547D47BC6D51EBF",
+# CHECK-NEXT:"name": "foo",
+# CHECK-NEXT:"usr": "c:@F@foo#"
+---
+{"jsonrpc":"2.0","method":"textDocument/didOpen","params":{"textDocument":{"uri":"test:///nested-decl.cpp","languageId":"cpp","version":1,"text":"namespace nnn { struct aaa {}; void foo() { aaa a; }; }"}}}
+---
+{"jsonrpc":"2.0","id":1,"method":"textDocument/cursorInfo","params":{"textDocument":{"uri":"test:///nested-decl.cpp"},"position":{"line":0,"character":46}}}
+# CHECK:"containerName": "nnn::",
+# CHECK-NEXT:"id": "20237FF18EB405D842456DC5D578426D",
+# CHECK-NEXT:"name": "aaa",
+# CHECK-NEXT:"usr": "c:@N@nnn@S@aaa"
+---
+{"jsonrpc":"2.0","method":"textDocument/didOpen","params":{"textDocument":{"uri":"test:///reference.cpp","languageId":"cpp","version":1,"text":"int value; void foo(int) {} void bar() { foo(value); }"}}}
+---
+{"jsonrpc":"2.0","id":1,"method":"textDocument/cursorInfo","params":{"textDocument":{"uri":"test:///reference.cpp"},"position":{"line":0,"character":48}}}
+# CHECK:"containerName": "",
+# CHECK-NEXT:"id": "844613FB2393C9D40A2AFF25D5D316A1",
+# CHECK-NEXT:"name": "value",
+# CHECK-NEXT:"usr": "c:@value"
+---
+{"jsonrpc":"2.0","method":"textDocument/didOpen","params":{"textDocument":{"uri":"test:///local.cpp","languageId":"cpp","version":1,"text":"void foo() { int aaa; int bbb = aaa; }"}}}
+---
+{"jsonrpc":"2.0","id":1,"method":"textDocument/cursorInfo","params":{"textDocument":{"uri":"test:///local.cpp"},"position":{"line":0,"character":33}}}
+# CHECK:"containerName": "",
+# CHECK-NEXT:"id": "C05589F2664B06F392C2C438568E55E0",
+# CHECK-NEXT:"name": "aaa",
+# CHECK-NEXT:"usr": "c:local.cpp@13@F@foo#@aaa"
+---
+{"jsonrpc":"2.0","method":"textDocument/didOpen","params":{"textDocument":{"uri":"test:///macro.cpp","languageId":"cpp","version":1,"text":"#define MACRO 5\nint i = MACRO;"}}}
+---
+{"jsonrpc":"2.0","id":1,"method":"textDocument/cursorInfo","params":{"textDocument":{"uri":"test:///macro.cpp"},"position":{"line":1,"character":11}}}
+# CHECK:"containerName": "",
+# CHECK-NEXT:"id": "29EB506CBDF1BA6D1B6EC203FF03B384",
+# CHECK-NEXT:"name": "MACRO",
+# CHECK-NEXT:"usr": "c:macro.cpp@24@macro@MACRO"
+---
+{"jsonrpc":"2.0","id":3,"method":"shutdown"}
+---
+{"jsonrpc":"2.0","method":"exit"}
Index: XRefs.h
===
--- XRefs.h
+++ XRefs.h
@@ -38,6 +38,9 @@
std::vector findReferences(ParsedAST &AST, Position Pos,
const Symbo
[PATCH] D54799: [clangd][WIP] textDocument/CursorInfo method
sammccall added a comment.
Thanks for sending this! Broadly looks good, a few details to work out. I think
the biggest one is multiple symbols which you've flagged.
> I'd like to ask for early feedback - what's still missing is relevant client
> capability.
I'm actually not 100% sure that's necessary for custom LSP methods.
- a server capability would suffice at least, unless we're worried clients are
going to call it "by mistake"?
- even that doesn't seem much better than just allowing the call where other
servers would return an error. Keep it simple?
I'd suggest leaving it out unless others feel strongly.
> Couple things that I'd love to hear opinions about are:
>
> - conditional return in `getCursorInfo` - Should we return for example data
> with empty `USR`?
Please return a symbol unless it has no SymbolID (we don't treat those as
symbols in clangd).
In practice this amounts to the same thing for now (SymbolID is derived from
USR). May change in the future though.
> - `containerName` of local variables - It's currently empty even if semantic
> parent has a name (say it's a function). (Please search for local.cpp in the
> test.) Is that what we want?
good question, no idea.
> - For now I used `getSymbolAtPosition()` as it's simpler and fits the context
> better. However I assume I could use this optimization from
> `tooling::getNamedDeclAt()` (in a separate patch):
> https://github.com/llvm-mirror/clang/blob/master/lib/Tooling/Refactoring/Rename/#L82
Yes, though at least for C++ this optimization won't usually buy anything, as
the top-level decl will just be a namespace decl I think.
We could plumb this deeper if we want to win more here.
- One thing I am wondering about is whether we could use (and whether it's a
significant improvement) some early return in `indexTopLevelDecls` (using
`DeclarationAndMacrosFinder`) also for hover and definition use-cases. Is it
correct to assume that at a given `Location` there can be maximum of one
declaration and one definition?
There can be multiple and we should return them all. (Not sure what we do for
hover, but defs handles this correctly).
I can't remember which are the important cases, but one we handle like this is
implicit constructor calls:
class Foo { Foo(const char*); }
void g(Foo);
const char* str = "abc";
g(str);
completion on the `s` in the last line finds both the Foo constructor and the
definition of `str`.
Maybe it's worth revisiting this if we can't find any really important
examples. That would involve digging up the historical context (I don't
remember it right now).
> P. S. Alex and Ben have thanksgiving break now so they'll probably add any
> feedback next week.
Hope they're having a good break!
Comment at: ClangdLSPServer.cpp:728
MsgHandler->bind("workspace/didChangeConfiguration",
&ClangdLSPServer::onChangeConfiguration);
+ MsgHandler->bind("textDocument/cursorInfo", &ClangdLSPServer::onCursorInfo);
// clang-format on
I'd suggest `textDocument/symbolInfo` for consistency with LSP (and similar
with internal names).
The term "symbol" is used throughout LSP to describe roughly what we're using
it to mean here.
Conversely I guess "cursor" refers to the editor cursor, but LSP seems to
prefer just talking about positions.
Comment at: ClangdServer.h:205
+ /// Get cursor info for given position.
+ void getCursorInfo(PathRef File, Position Pos,
+ Callback> CB);
nit: just `cursorInfo` (or `symbolInfo` etc) for consistency with other where
there isn't a specific verb.
Comment at: Protocol.cpp:436
+ const IdentifierData &SUCI) {
+ O << SUCI.containerName << "::" << SUCI.name << " - " << toJSON(SUCI);
+ return O;
(you only want containername if non-empty, right?)
Comment at: Protocol.h:676
+/// Represents information about identifier.
+struct IdentifierData {
Unless I'm misunderstanding, this is about a symbol, not an identifier. (e.g.
overloads of C++ functions are distinct symbols). Maybe `SymbolDetails` would
be a useful name.
There's a question of scope here: `getCursorInfo` seems to return a single
instance of these, but there can be 0, 1, or many symbols at a given location.
These could be `vector` or so, or if this struct is intended to
encapsulate them all, it could be named `SymbolsAtLocation` or so.
Comment at: Protocol.h:676
+/// Represents information about identifier.
+struct IdentifierData {
sammccall wrote:
> Unless I'm misunderstanding, this is about a symbol, not an identifier. (e.g.
> overloads of C++ functions are distinct symbols). Maybe `SymbolDetails` would
> be a useful name.
>
> There's a question of scope here: `getCursorInfo` seems to return a single
> instance of these, but there can be 0, 1, or many symbols at a given
>
[PATCH] D54799: [clangd][WIP] textDocument/CursorInfo method
jkorous marked 7 inline comments as done.
jkorous added a comment.
Hi Sam.
In https://reviews.llvm.org/D54799#1305470, @sammccall wrote:
>
>> I'd like to ask for early feedback - what's still missing is relevant client
>> capability.
>
> I'd suggest leaving it out unless others feel strongly.
More than happy to keep it simple! I somehow assumed there'll be demand for
this capability.
>> - conditional return in `getCursorInfo` - Should we return for example data
>> with empty `USR`?
>
> Please return a symbol unless it has no SymbolID (we don't treat those as
> symbols in clangd).
Ok.
>> - `containerName` of local variables - It's currently empty even if semantic
>> parent has a name (say it's a function). (Please search for local.cpp in the
>> test.) Is that what we want?
>
> good question, no idea.
I am adding an attempt to get a named declaration context for such cases. It's
easy to remove if we decide against that.
>> - For now I used `getSymbolAtPosition()` as it's simpler and fits the
>> context better. However I assume I could use this optimization from
>> `tooling::getNamedDeclAt()` (in a separate patch):
>> https://github.com/llvm-mirror/clang/blob/master/lib/Tooling/Refactoring/Rename/#L82
>
> Yes, though at least for C++ this optimization won't usually buy anything, as
> the top-level decl will just be a namespace decl I think.
> We could plumb this deeper if we want to win more here.
Ok, I'll leave it for the hypothetical future.
> - One thing I am wondering about is whether we could use (and whether it's a
> significant improvement) some early return in `indexTopLevelDecls` (using
> `DeclarationAndMacrosFinder`) also for hover and definition use-cases. Is it
> correct to assume that at a given `Location` there can be maximum of one
> declaration and one definition? There can be multiple and we should return
> them all. (Not sure what we do for hover, but defs handles this correctly).
Ah, my bad - I was considering only explicit cases. Thanks for the explanation.
In the context of textDocument/cursorInfo - does it make sense to use the first
explicit declaration we find or am I missing something still? (This is related
to discussion about `StopOnFirstDeclFound`.)
I processed and implemented most of your comments. Two major tasks are:
finishing the discussion about multiple declarations and writing unittests
(preferably after we agree on interfaces).
Comment at: Protocol.h:676
+/// Represents information about identifier.
+struct IdentifierData {
sammccall wrote:
> sammccall wrote:
> > Unless I'm misunderstanding, this is about a symbol, not an identifier.
> > (e.g. overloads of C++ functions are distinct symbols). Maybe
> > `SymbolDetails` would be a useful name.
> >
> > There's a question of scope here: `getCursorInfo` seems to return a single
> > instance of these, but there can be 0, 1, or many symbols at a given
> > location. These could be `vector` or so, or if this struct
> > is intended to encapsulate them all, it could be named `SymbolsAtLocation`
> > or so.
> /// This is returned from textDocument/cursorInfo, which is a clangd
> extension.
I probably don't understand "many symbols at a given location" - more at the
end of my out-of-line comment.
Comment at: Protocol.h:678
+struct IdentifierData {
+ /// The name of this symbol.
+ std::string name;
sammccall wrote:
> This comment doesn't really say anything. It can be omitted, or we could add
> additional information.
> (Some of the existing comments are equally content-free, because we've copied
> the comments from LSP verbatim, but that's not the case here)
I was actually wondering what is the value of those comments but decided to
keep it consistent. Deleting.
Comment at: Protocol.h:682
+ /// The name of the symbol containing this symbol.
+ std::string containerName;
+
sammccall wrote:
> sammccall wrote:
> > If we're not going to merge, I'd suggest making this "scope" and allowing
> > it to end with ::.
> > This makes it easier to compose (qname = scope + name), and makes it
> > clearer what "contains" means. (LSP got this wrong IMO, at least for C++).
> what do you want this to do for objc methods and fields?
> It'd be worth having a test if it's important, I'm guilty of not
> knowing/checking ObjC behavior.
I don't really have any opinion about this but @benlangmuir might have.
I'd just make sure whatever we decide makes sense for local symbols (if we keep
returning them).
I'll add tests for ObjC. Thanks for pointing that out!
Comment at: Protocol.h:684
+
+ /// The USR of this symbol.
+ llvm::SmallString<128> USR;
sammccall wrote:
> USR could use some definition/references.
True. Would you go further than expanding the acronym and copying description
from doxygen annotation of `SymbolID`?
I haven't actually found any bette
[PATCH] D54799: [clangd][WIP] textDocument/CursorInfo method
jkorous updated this revision to Diff 175069.
jkorous marked 2 inline comments as done.
jkorous added a comment.
Changes based on review.
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D54799
Files:
ClangdLSPServer.cpp
ClangdLSPServer.h
ClangdServer.cpp
ClangdServer.h
Protocol.cpp
Protocol.h
XRefs.cpp
XRefs.h
clangd/cursor-info.test
index/Index.cpp
index/Index.h
Index: clangd/cursor-info.test
===
--- /dev/null
+++ clangd/cursor-info.test
@@ -0,0 +1,46 @@
+# RUN: clangd -lit-test < %s | FileCheck %s
+{"jsonrpc":"2.0","id":0,"method":"initialize","params":{"processId":123,"rootPath":"clangd","capabilities":{},"trace":"off"}}
+---
+{"jsonrpc":"2.0","method":"textDocument/didOpen","params":{"textDocument":{"uri":"test:///simple.cpp","languageId":"cpp","version":1,"text":"void foo(); int main() { foo(); }\n"}}}
+---
+{"jsonrpc":"2.0","id":1,"method":"textDocument/symbolInfo","params":{"textDocument":{"uri":"test:///simple.cpp"},"position":{"line":0,"character":27}}}
+# CHECK:"containerName": "",
+# CHECK-NEXT:"id": "CA2EBE44A1D76D2A1547D47BC6D51EBF",
+# CHECK-NEXT:"name": "foo",
+# CHECK-NEXT:"usr": "c:@F@foo#"
+---
+{"jsonrpc":"2.0","method":"textDocument/didOpen","params":{"textDocument":{"uri":"test:///nested-decl.cpp","languageId":"cpp","version":1,"text":"namespace nnn { struct aaa {}; void foo() { aaa a; }; }"}}}
+---
+{"jsonrpc":"2.0","id":1,"method":"textDocument/symbolInfo","params":{"textDocument":{"uri":"test:///nested-decl.cpp"},"position":{"line":0,"character":46}}}
+# CHECK:"containerName": "nnn::",
+# CHECK-NEXT:"id": "20237FF18EB405D842456DC5D578426D",
+# CHECK-NEXT:"name": "aaa",
+# CHECK-NEXT:"usr": "c:@N@nnn@S@aaa"
+---
+{"jsonrpc":"2.0","method":"textDocument/didOpen","params":{"textDocument":{"uri":"test:///reference.cpp","languageId":"cpp","version":1,"text":"int value; void foo(int) {} void bar() { foo(value); }"}}}
+---
+{"jsonrpc":"2.0","id":1,"method":"textDocument/symbolInfo","params":{"textDocument":{"uri":"test:///reference.cpp"},"position":{"line":0,"character":48}}}
+# CHECK:"containerName": "",
+# CHECK-NEXT:"id": "844613FB2393C9D40A2AFF25D5D316A1",
+# CHECK-NEXT:"name": "value",
+# CHECK-NEXT:"usr": "c:@value"
+---
+{"jsonrpc":"2.0","method":"textDocument/didOpen","params":{"textDocument":{"uri":"test:///local.cpp","languageId":"cpp","version":1,"text":"void foo() { int aaa; int bbb = aaa; }"}}}
+---
+{"jsonrpc":"2.0","id":1,"method":"textDocument/symbolInfo","params":{"textDocument":{"uri":"test:///local.cpp"},"position":{"line":0,"character":33}}}
+# CHECK:"containerName": "foo",
+# CHECK-NEXT:"id": "C05589F2664B06F392C2C438568E55E0",
+# CHECK-NEXT:"name": "aaa",
+# CHECK-NEXT:"usr": "c:local.cpp@13@F@foo#@aaa"
+---
+{"jsonrpc":"2.0","method":"textDocument/didOpen","params":{"textDocument":{"uri":"test:///macro.cpp","languageId":"cpp","version":1,"text":"#define MACRO 5\nint i = MACRO;"}}}
+---
+{"jsonrpc":"2.0","id":1,"method":"textDocument/symbolInfo","params":{"textDocument":{"uri":"test:///macro.cpp"},"position":{"line":1,"character":11}}}
+# CHECK:"containerName": "",
+# CHECK-NEXT:"id": "29EB506CBDF1BA6D1B6EC203FF03B384",
+# CHECK-NEXT:"name": "MACRO",
+# CHECK-NEXT:"usr": "c:macro.cpp@24@macro@MACRO"
+---
+{"jsonrpc":"2.0","id":3,"method":"shutdown"}
+---
+{"jsonrpc":"2.0","method":"exit"}
Index: index/Index.h
===
--- index/Index.h
+++ index/Index.h
@@ -10,11 +10,11 @@
#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANGD_INDEX_INDEX_H
#define LLVM_CLANG_TOOLS_EXTRA_CLANGD_INDEX_INDEX_H
+#include "Protocol.h"
#include "clang/Index/IndexSymbol.h"
#include "clang/Lex/Lexer.h"
#include "llvm/ADT/DenseMap.h"
#include "llvm/ADT/DenseSet.h"
-#include "llvm/ADT/Hashing.h"
#include "llvm/ADT/Optional.h"
#include "llvm/ADT/SmallVector.h"
#include "llvm/ADT/StringExtras.h"
@@ -94,54 +94,6 @@
}
llvm::raw_ostream &operator<<(llvm::raw_ostream &, const SymbolLocation &);
-// The class identifies a particular C++ symbol (class, function, method, etc).
-//
-// As USRs (Unified Symbol Resolution) could be large, especially for functions
-// with long type arguments, SymbolID is using truncated SHA1(USR) values to
-// guarantee the uniqueness of symbols while using a relatively small amount of
-// memory (vs storing USRs directly).
-//
-// SymbolID can be used as key in the symbol indexes to lookup the symbol.
-class SymbolID {
-public:
- SymbolID() = default;
- explicit SymbolID(llvm::StringRef USR);
-
- bool operator==(const SymbolID &Sym) const {
-return HashValue == Sym.HashValue;
- }
- bool operator<(const SymbolID &Sym) const {
-return HashValue < Sym.HashValue;
- }
-
- // The stored hash is truncated to RawSize bytes.
- // This trades off memory against the number of symbols we can handle.
- /
[PATCH] D54799: [clangd][WIP] textDocument/CursorInfo method
sammccall added a subscriber: hokein. sammccall added a comment. In https://reviews.llvm.org/D54799#1306488, @jkorous wrote: > >> - conditional return in `getCursorInfo` - Should we return for example > >> data with empty `USR`? > > > > Please return a symbol unless it has no SymbolID (we don't treat those as > > symbols in clangd). > > Ok. Sorry, this was a mistake - we do in fact use decls that don't have symbol IDs (we just don't look them up in the index). So I think both SymbolID and USR are optional. Comment at: ClangdServer.h:205 + /// Get cursor info for given position. + void getCursorInfo(PathRef File, Position Pos, + Callback> CB); sammccall wrote: > nit: just `cursorInfo` (or `symbolInfo` etc) for consistency with other where > there isn't a specific verb. (not done I think? Still says `getSymbolInfo`) Comment at: Protocol.h:684 + + /// The USR of this symbol. + llvm::SmallString<128> USR; jkorous wrote: > sammccall wrote: > > USR could use some definition/references. > True. Would you go further than expanding the acronym and copying description > from doxygen annotation of `SymbolID`? > I haven't actually found any better description in clang repository. > > Shall I note `include/clang/Index/USRGeneration.h`? I'd add something like: ``` This is an opaque string uniquely identifying a symbol. Unlike SymbolID, it is variable-length and somewhat human-readable. It is a common representation across several clang tools (See USRGeneration.h) ``` Comment at: Protocol.h:689 + /// Size is dependent on clang::clangd::SymbolID::RawValue. + llvm::SmallString<32> ID; +}; jkorous wrote: > sammccall wrote: > > can this just be SymbolID? > I didn't want to pull `SymbolID` from `index` originally but sure. > > Are you fine with it living in `Protocol.h` or shall I move it/factor it out? SymbolID shouldn't live in `Protocol.h`. It hadn't occurred to me, but including all of `Index/Index.h` is risky here - it's pretty broad in scope and we run the risk of a circular dependency down the line. Sorry for not noticing! I'm fine with any of: - pull out SymbolID into `Index/SymbolID.h` which would have few dependencies - revert to using std::string here as you originally did - depend on index.h for now, and solve this when it becomes a problem Comment at: Protocol.h:690 + llvm::SmallString<32> ID; +}; +llvm::json::Value toJSON(const IdentifierData &); jkorous wrote: > sammccall wrote: > > I think SymbolKind would be useful (in particular distinguishing macros > > might be important?) > > > > This does point towards maybe adding to SymbolInformation rather than a new > > struct, but I'm not sure which is best. > AFAIK for our use-case the information in USR is good enough (e. g. > "c:foo.cpp@24@macro@FOO") so we might wait until we have a real need. WDYT? I would personally advise against parsing information out of USRs, but up to you. (On the other hand if you're just using them as keys to look up information from elsewhere, that makes sense) Comment at: XRefs.cpp:95 Preprocessor &PP; + const bool StopOnFirstDeclFound; jkorous wrote: > sammccall wrote: > > Please don't do this - it's inconsistent with the other XRefs features. > > (If for some reason you *really* want just one result, then this is easy to > > do on the client side if we get the API right) > Sure, let's discuss. I probably got confused by your suggestion of using > `getSymbolAtPosition()` in https://reviews.llvm.org/D54529#1299531. > > Do I understand it correctly that you mean visiting all declarations and > selecting the single explicit one in `getSymbolInfo()`? > Or do you actually mean change of interface such as this? > `llvm::Optional getSymbolInfo()` -> > `std::vector getSymbolInfo()`. > Is my assumption that there could be only single explicit declaration for any > given position correct in the first place? I looked into this, and the multiple-symbols is more of a mess than I thought it was. At a high level: I think it's important that various features that use "symbol under cursor" are consistent. If you do defs or refs or highlights at a point, you should be querying the same set of symbols. And the same goes for anything that starts with a symbolInfo call. Unfortunately the way the sorting was introduced in rL341463 meant the existing things are not quite consistent, which confuses things a bit. But at a high level: - the status quo is that findDefinition/findRefs/highlights deal with multiple symbols. So yes, `symbolInfo` should return a `vector` as things stand. - the multiple-symbols situation is a bit surprising and maybe not that useful. It might be possible to change this behavior. It should be changed for all methods (i.e. either we land this patch with `vector` and then (brea
[PATCH] D54799: [clangd][WIP] textDocument/CursorInfo method
hokein added inline comments.
Comment at: XRefs.cpp:95
Preprocessor &PP;
+ const bool StopOnFirstDeclFound;
sammccall wrote:
> jkorous wrote:
> > sammccall wrote:
> > > Please don't do this - it's inconsistent with the other XRefs features.
> > > (If for some reason you *really* want just one result, then this is easy
> > > to do on the client side if we get the API right)
> > Sure, let's discuss. I probably got confused by your suggestion of using
> > `getSymbolAtPosition()` in https://reviews.llvm.org/D54529#1299531.
> >
> > Do I understand it correctly that you mean visiting all declarations and
> > selecting the single explicit one in `getSymbolInfo()`?
> > Or do you actually mean change of interface such as this?
> > `llvm::Optional getSymbolInfo()` ->
> > `std::vector getSymbolInfo()`.
> > Is my assumption that there could be only single explicit declaration for
> > any given position correct in the first place?
> I looked into this, and the multiple-symbols is more of a mess than I thought
> it was.
>
> At a high level: I think it's important that various features that use
> "symbol under cursor" are consistent. If you do defs or refs or highlights at
> a point, you should be querying the same set of symbols. And the same goes
> for anything that starts with a symbolInfo call.
>
> Unfortunately the way the sorting was introduced in rL341463 meant the
> existing things are not quite consistent, which confuses things a bit.
>
> But at a high level:
> - the status quo is that findDefinition/findRefs/highlights deal with
> multiple symbols. So yes, `symbolInfo` should return a
> `vector` as things stand.
> - the multiple-symbols situation is a bit surprising and maybe not that
> useful. It might be possible to change this behavior. It should be changed
> for all methods (i.e. either we land this patch with `vector`
> and then (breaking) change it to `Optional` later, or we
> remove the multiple-symbols behavior first.
> - I don't know what the deal is with "explicit". The cases with multiple
> symbols are already obscure, cases with multiple explicit symbols are
> probably even rarer if they exist. But I don't think it's particularly safe
> to assume they don't. And getSymbolInfo really shouldn't be adding its own
> unique logic around this.
>
> @hokein @ilya-biryukov We should chat about the historical context here.
I have the same question when I first touched this code :)
IIRC, we want all symbols in GoToDef (as we can show multiple symbols to
clients). Unfortunately, there are some inconsistencies between `def` (return
all), `ref` (only return explicit symbols), `hover` (return the first one).
I agree that most cases are single-symbol, but the typical case with
multiple-symbols is below. Personally, I'm +1 on removing the multiple-symbols
behavior, it can simplify many things...
```
namespace ns {
void foo(int);
void foo(double);
}
using ns::f^oo;
```
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D54799
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D54799: [clangd][WIP] textDocument/CursorInfo method
jkorous marked 7 inline comments as done. jkorous added a comment. In https://reviews.llvm.org/D54799#1306585, @sammccall wrote: > So I think both SymbolID and USR are optional. No problem. I am just wondering if it make sense to include any symbol with empty name, empty USR and no ID in LSP response. I assume either clangd or our LSP client (and hypothetically others too) will have to filter these out. But it might violate consistency with other LSP methods. Anyway, if we agree on filtering such symbols either in `getSymbolInfo()` or `ClangdServer::getSymbolInfo()` I am happy to do it, if not it's fine with me. Comment at: ClangdServer.h:205 + /// Get cursor info for given position. + void getCursorInfo(PathRef File, Position Pos, + Callback> CB); sammccall wrote: > sammccall wrote: > > nit: just `cursorInfo` (or `symbolInfo` etc) for consistency with other > > where there isn't a specific verb. > (not done I think? Still says `getSymbolInfo`) Sorry, my bad - I missed the verb dropping. Fixing now. BTW if we want consistency here we should probably rename `findReferences()`, `findHover()` too. Comment at: Protocol.h:689 + /// Size is dependent on clang::clangd::SymbolID::RawValue. + llvm::SmallString<32> ID; +}; sammccall wrote: > jkorous wrote: > > sammccall wrote: > > > can this just be SymbolID? > > I didn't want to pull `SymbolID` from `index` originally but sure. > > > > Are you fine with it living in `Protocol.h` or shall I move it/factor it > > out? > SymbolID shouldn't live in `Protocol.h`. > > It hadn't occurred to me, but including all of `Index/Index.h` is risky here > - it's pretty broad in scope and we run the risk of a circular dependency > down the line. Sorry for not noticing! > > I'm fine with any of: > - pull out SymbolID into `Index/SymbolID.h` which would have few dependencies > - revert to using std::string here as you originally did > - depend on index.h for now, and solve this when it becomes a problem To me the cleanest solution seems to be `Index/SymbolID.h`. Generally if we ever get to a point where we'd hit this kind of issues more often it might be worthwhile to keep data structures for LSP protocol and for server implementation separate. But I don't think we're anywhere close yet. Comment at: Protocol.h:690 + llvm::SmallString<32> ID; +}; +llvm::json::Value toJSON(const IdentifierData &); sammccall wrote: > jkorous wrote: > > sammccall wrote: > > > I think SymbolKind would be useful (in particular distinguishing macros > > > might be important?) > > > > > > This does point towards maybe adding to SymbolInformation rather than a > > > new struct, but I'm not sure which is best. > > AFAIK for our use-case the information in USR is good enough (e. g. > > "c:foo.cpp@24@macro@FOO") so we might wait until we have a real need. WDYT? > I would personally advise against parsing information out of USRs, but up to > you. > (On the other hand if you're just using them as keys to look up information > from elsewhere, that makes sense) I assume that's what we do but I'll let @benlangmuir or @arphaman confirm this. Comment at: XRefs.cpp:95 Preprocessor &PP; + const bool StopOnFirstDeclFound; hokein wrote: > sammccall wrote: > > jkorous wrote: > > > sammccall wrote: > > > > Please don't do this - it's inconsistent with the other XRefs features. > > > > (If for some reason you *really* want just one result, then this is > > > > easy to do on the client side if we get the API right) > > > Sure, let's discuss. I probably got confused by your suggestion of using > > > `getSymbolAtPosition()` in https://reviews.llvm.org/D54529#1299531. > > > > > > Do I understand it correctly that you mean visiting all declarations and > > > selecting the single explicit one in `getSymbolInfo()`? > > > Or do you actually mean change of interface such as this? > > > `llvm::Optional getSymbolInfo()` -> > > > `std::vector getSymbolInfo()`. > > > Is my assumption that there could be only single explicit declaration for > > > any given position correct in the first place? > > I looked into this, and the multiple-symbols is more of a mess than I > > thought it was. > > > > At a high level: I think it's important that various features that use > > "symbol under cursor" are consistent. If you do defs or refs or highlights > > at a point, you should be querying the same set of symbols. And the same > > goes for anything that starts with a symbolInfo call. > > > > Unfortunately the way the sorting was introduced in rL341463 meant the > > existing things are not quite consistent, which confuses things a bit. > > > > But at a high level: > > - the status quo is that findDefinition/findRefs/highlights deal with > > multiple symbols. So yes, `symbolInfo` should return a > > `vector` as things stand. > > - th
[PATCH] D54799: [clangd][WIP] textDocument/CursorInfo method
jkorous updated this revision to Diff 175115.
jkorous marked 2 inline comments as done.
jkorous added a comment.
Herald added a subscriber: mgorny.
Couple minor changes based on discussion.
- Move `SymbolID` to `index/SymbolID.h`.
- Rename in `ClangdServer` - drop the verb from method name.
- Remove conditional return in `XRefs.cpp`.
https://reviews.llvm.org/D54799
Files:
clangd/CMakeLists.txt
clangd/ClangdLSPServer.cpp
clangd/ClangdLSPServer.h
clangd/ClangdServer.cpp
clangd/ClangdServer.h
clangd/Protocol.cpp
clangd/Protocol.h
clangd/XRefs.cpp
clangd/XRefs.h
clangd/index/Index.cpp
clangd/index/Index.h
clangd/index/SymbolID.cpp
clangd/index/SymbolID.h
test/clangd/cursor-info.test
Index: test/clangd/cursor-info.test
===
--- /dev/null
+++ test/clangd/cursor-info.test
@@ -0,0 +1,46 @@
+# RUN: clangd -lit-test < %s | FileCheck %s
+{"jsonrpc":"2.0","id":0,"method":"initialize","params":{"processId":123,"rootPath":"clangd","capabilities":{},"trace":"off"}}
+---
+{"jsonrpc":"2.0","method":"textDocument/didOpen","params":{"textDocument":{"uri":"test:///simple.cpp","languageId":"cpp","version":1,"text":"void foo(); int main() { foo(); }\n"}}}
+---
+{"jsonrpc":"2.0","id":1,"method":"textDocument/symbolInfo","params":{"textDocument":{"uri":"test:///simple.cpp"},"position":{"line":0,"character":27}}}
+# CHECK:"containerName": "",
+# CHECK-NEXT:"id": "CA2EBE44A1D76D2A1547D47BC6D51EBF",
+# CHECK-NEXT:"name": "foo",
+# CHECK-NEXT:"usr": "c:@F@foo#"
+---
+{"jsonrpc":"2.0","method":"textDocument/didOpen","params":{"textDocument":{"uri":"test:///nested-decl.cpp","languageId":"cpp","version":1,"text":"namespace nnn { struct aaa {}; void foo() { aaa a; }; }"}}}
+---
+{"jsonrpc":"2.0","id":1,"method":"textDocument/symbolInfo","params":{"textDocument":{"uri":"test:///nested-decl.cpp"},"position":{"line":0,"character":46}}}
+# CHECK:"containerName": "nnn::",
+# CHECK-NEXT:"id": "20237FF18EB405D842456DC5D578426D",
+# CHECK-NEXT:"name": "aaa",
+# CHECK-NEXT:"usr": "c:@N@nnn@S@aaa"
+---
+{"jsonrpc":"2.0","method":"textDocument/didOpen","params":{"textDocument":{"uri":"test:///reference.cpp","languageId":"cpp","version":1,"text":"int value; void foo(int) {} void bar() { foo(value); }"}}}
+---
+{"jsonrpc":"2.0","id":1,"method":"textDocument/symbolInfo","params":{"textDocument":{"uri":"test:///reference.cpp"},"position":{"line":0,"character":48}}}
+# CHECK:"containerName": "",
+# CHECK-NEXT:"id": "844613FB2393C9D40A2AFF25D5D316A1",
+# CHECK-NEXT:"name": "value",
+# CHECK-NEXT:"usr": "c:@value"
+---
+{"jsonrpc":"2.0","method":"textDocument/didOpen","params":{"textDocument":{"uri":"test:///local.cpp","languageId":"cpp","version":1,"text":"void foo() { int aaa; int bbb = aaa; }"}}}
+---
+{"jsonrpc":"2.0","id":1,"method":"textDocument/symbolInfo","params":{"textDocument":{"uri":"test:///local.cpp"},"position":{"line":0,"character":33}}}
+# CHECK:"containerName": "foo",
+# CHECK-NEXT:"id": "C05589F2664B06F392C2C438568E55E0",
+# CHECK-NEXT:"name": "aaa",
+# CHECK-NEXT:"usr": "c:local.cpp@13@F@foo#@aaa"
+---
+{"jsonrpc":"2.0","method":"textDocument/didOpen","params":{"textDocument":{"uri":"test:///macro.cpp","languageId":"cpp","version":1,"text":"#define MACRO 5\nint i = MACRO;"}}}
+---
+{"jsonrpc":"2.0","id":1,"method":"textDocument/symbolInfo","params":{"textDocument":{"uri":"test:///macro.cpp"},"position":{"line":1,"character":11}}}
+# CHECK:"containerName": "",
+# CHECK-NEXT:"id": "29EB506CBDF1BA6D1B6EC203FF03B384",
+# CHECK-NEXT:"name": "MACRO",
+# CHECK-NEXT:"usr": "c:macro.cpp@24@macro@MACRO"
+---
+{"jsonrpc":"2.0","id":3,"method":"shutdown"}
+---
+{"jsonrpc":"2.0","method":"exit"}
Index: clangd/index/SymbolID.h
===
--- /dev/null
+++ clangd/index/SymbolID.h
@@ -0,0 +1,66 @@
+//===--- SymbolID.h --*- C++-*-===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===--===//
+
+#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANGD_INDEX_SYMBOLID_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANGD_INDEX_SYMBOLID_H
+
+#include "llvm/ADT/StringRef.h"
+#include "llvm/Support/Error.h"
+#include "llvm/Support/raw_ostream.h"
+#include
+#include
+
+namespace clang {
+namespace clangd {
+
+// The class identifies a particular C++ symbol (class, function, method, etc).
+//
+// As USRs (Unified Symbol Resolution) could be large, especially for functions
+// with long type arguments, SymbolID is using truncated SHA1(USR) values to
+// guarantee the uniqueness of symbols while using a relatively small amount of
+// memory (vs storing USRs directly).
+//
+// SymbolID can
[PATCH] D54799: [clangd][WIP] textDocument/CursorInfo method
jkorous updated this revision to Diff 175137.
jkorous added a comment.
Multiple symbols per location.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D54799/new/
https://reviews.llvm.org/D54799
Files:
clangd/CMakeLists.txt
clangd/ClangdLSPServer.cpp
clangd/ClangdLSPServer.h
clangd/ClangdServer.cpp
clangd/ClangdServer.h
clangd/Protocol.cpp
clangd/Protocol.h
clangd/XRefs.cpp
clangd/XRefs.h
clangd/index/Index.cpp
clangd/index/Index.h
clangd/index/SymbolID.cpp
clangd/index/SymbolID.h
test/clangd/cursor-info.test
Index: test/clangd/cursor-info.test
===
--- /dev/null
+++ test/clangd/cursor-info.test
@@ -0,0 +1,46 @@
+# RUN: clangd -lit-test < %s | FileCheck %s
+{"jsonrpc":"2.0","id":0,"method":"initialize","params":{"processId":123,"rootPath":"clangd","capabilities":{},"trace":"off"}}
+---
+{"jsonrpc":"2.0","method":"textDocument/didOpen","params":{"textDocument":{"uri":"test:///simple.cpp","languageId":"cpp","version":1,"text":"void foo(); int main() { foo(); }\n"}}}
+---
+{"jsonrpc":"2.0","id":1,"method":"textDocument/symbolInfo","params":{"textDocument":{"uri":"test:///simple.cpp"},"position":{"line":0,"character":27}}}
+# CHECK:"containerName": "",
+# CHECK-NEXT:"id": "CA2EBE44A1D76D2A1547D47BC6D51EBF",
+# CHECK-NEXT:"name": "foo",
+# CHECK-NEXT:"usr": "c:@F@foo#"
+---
+{"jsonrpc":"2.0","method":"textDocument/didOpen","params":{"textDocument":{"uri":"test:///nested-decl.cpp","languageId":"cpp","version":1,"text":"namespace nnn { struct aaa {}; void foo() { aaa a; }; }"}}}
+---
+{"jsonrpc":"2.0","id":1,"method":"textDocument/symbolInfo","params":{"textDocument":{"uri":"test:///nested-decl.cpp"},"position":{"line":0,"character":46}}}
+# CHECK:"containerName": "nnn::",
+# CHECK-NEXT:"id": "20237FF18EB405D842456DC5D578426D",
+# CHECK-NEXT:"name": "aaa",
+# CHECK-NEXT:"usr": "c:@N@nnn@S@aaa"
+---
+{"jsonrpc":"2.0","method":"textDocument/didOpen","params":{"textDocument":{"uri":"test:///reference.cpp","languageId":"cpp","version":1,"text":"int value; void foo(int) {} void bar() { foo(value); }"}}}
+---
+{"jsonrpc":"2.0","id":1,"method":"textDocument/symbolInfo","params":{"textDocument":{"uri":"test:///reference.cpp"},"position":{"line":0,"character":48}}}
+# CHECK:"containerName": "",
+# CHECK-NEXT:"id": "844613FB2393C9D40A2AFF25D5D316A1",
+# CHECK-NEXT:"name": "value",
+# CHECK-NEXT:"usr": "c:@value"
+---
+{"jsonrpc":"2.0","method":"textDocument/didOpen","params":{"textDocument":{"uri":"test:///local.cpp","languageId":"cpp","version":1,"text":"void foo() { int aaa; int bbb = aaa; }"}}}
+---
+{"jsonrpc":"2.0","id":1,"method":"textDocument/symbolInfo","params":{"textDocument":{"uri":"test:///local.cpp"},"position":{"line":0,"character":33}}}
+# CHECK:"containerName": "foo",
+# CHECK-NEXT:"id": "C05589F2664B06F392C2C438568E55E0",
+# CHECK-NEXT:"name": "aaa",
+# CHECK-NEXT:"usr": "c:local.cpp@13@F@foo#@aaa"
+---
+{"jsonrpc":"2.0","method":"textDocument/didOpen","params":{"textDocument":{"uri":"test:///macro.cpp","languageId":"cpp","version":1,"text":"#define MACRO 5\nint i = MACRO;"}}}
+---
+{"jsonrpc":"2.0","id":1,"method":"textDocument/symbolInfo","params":{"textDocument":{"uri":"test:///macro.cpp"},"position":{"line":1,"character":11}}}
+# CHECK:"containerName": "",
+# CHECK-NEXT:"id": "29EB506CBDF1BA6D1B6EC203FF03B384",
+# CHECK-NEXT:"name": "MACRO",
+# CHECK-NEXT:"usr": "c:macro.cpp@24@macro@MACRO"
+---
+{"jsonrpc":"2.0","id":3,"method":"shutdown"}
+---
+{"jsonrpc":"2.0","method":"exit"}
Index: clangd/index/SymbolID.h
===
--- /dev/null
+++ clangd/index/SymbolID.h
@@ -0,0 +1,66 @@
+//===--- SymbolID.h --*- C++-*-===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===--===//
+
+#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANGD_INDEX_SYMBOLID_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANGD_INDEX_SYMBOLID_H
+
+#include "llvm/ADT/StringRef.h"
+#include "llvm/Support/Error.h"
+#include "llvm/Support/raw_ostream.h"
+#include
+#include
+
+namespace clang {
+namespace clangd {
+
+// The class identifies a particular C++ symbol (class, function, method, etc).
+//
+// As USRs (Unified Symbol Resolution) could be large, especially for functions
+// with long type arguments, SymbolID is using truncated SHA1(USR) values to
+// guarantee the uniqueness of symbols while using a relatively small amount of
+// memory (vs storing USRs directly).
+//
+// SymbolID can be used as key in the symbol indexes to lookup the symbol.
+class SymbolID {
+public:
+ SymbolID() = default;
+ explicit SymbolID(llvm::StringRef USR);
+
+ bool oper
[PATCH] D54799: [clangd][WIP] textDocument/CursorInfo method
sammccall added a comment.
Thanks, this looks good! Just nits, and please do port most of the test cases
to unit tests.
Comment at: clangd/ClangdServer.cpp:529
+
+ WorkScheduler.runWithAST("CursorInfo", File, Bind(Action, std::move(CB)));
+}
nit: SymbolInfo
Comment at: clangd/Protocol.cpp:429
+ return json::Object{
+ {"name", P.name},
+ {"containerName", P.containerName},
for each of the attributes that can be logically absent, we should probably
serialize it conditionally (or serialize it as null).
We're usually happy enough to use sentinel values like "" to mean missing
internally, but we try not to send them over the wire.
(SymbolInformation isn't a great example unfortunately...)
Comment at: clangd/Protocol.h:691
+
+ SymbolID ID;
+};
Optional?
"" is a reasonable "absent" value for strings, but we don't particularly have
one for symbolID
Comment at: clangd/XRefs.cpp:771
+ContainerNameRef.consume_back("::");
+ } else {
+const auto *DC = ND->getDeclContext();
nit: just `else if (const auto* ParentND =
dyn_cast_or_null(ND->getDeclContext())`?
Comment at: clangd/XRefs.cpp:785
+}
+Results.emplace_back(std::move(NewSymbol));
+ }
nit: push_back (and below)
Comment at: clangd/index/SymbolID.h:58
+
+llvm::hash_code hash_value(const SymbolID &ID);
+
nit: missing include of llvm/ADT/Hashing.h
Comment at: test/clangd/cursor-info.test:1
+# RUN: clangd -lit-test < %s | FileCheck %s
+{"jsonrpc":"2.0","id":0,"method":"initialize","params":{"processId":123,"rootPath":"clangd","capabilities":{},"trace":"off"}}
(this file should be renamed)
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D54799/new/
https://reviews.llvm.org/D54799
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D54799: [clangd][WIP] textDocument/CursorInfo method
jkorous marked 8 inline comments as done. jkorous added a comment. Thank you very much for the review Sam! I am going to write proper unit tests and then just wait for Alex and Ben to take a look. Comment at: clangd/XRefs.cpp:785 +} +Results.emplace_back(std::move(NewSymbol)); + } sammccall wrote: > nit: push_back (and below) Sure, no problem. I have to admit I thought these two are the same but I guess you pointed this out because there's a functional difference. I'd appreciate if you could advise where can I learn the details. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D54799/new/ https://reviews.llvm.org/D54799 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D54799: [clangd][WIP] textDocument/CursorInfo method
jkorous updated this revision to Diff 175229.
jkorous marked an inline comment as done.
jkorous added a comment.
Addressed comments from the reivew.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D54799/new/
https://reviews.llvm.org/D54799
Files:
clangd/CMakeLists.txt
clangd/ClangdLSPServer.cpp
clangd/ClangdLSPServer.h
clangd/ClangdServer.cpp
clangd/ClangdServer.h
clangd/Protocol.cpp
clangd/Protocol.h
clangd/XRefs.cpp
clangd/XRefs.h
clangd/index/Index.cpp
clangd/index/Index.h
clangd/index/SymbolID.cpp
clangd/index/SymbolID.h
test/clangd/symbol-info.test
Index: test/clangd/symbol-info.test
===
--- /dev/null
+++ test/clangd/symbol-info.test
@@ -0,0 +1,46 @@
+# RUN: clangd -lit-test < %s | FileCheck %s
+{"jsonrpc":"2.0","id":0,"method":"initialize","params":{"processId":123,"rootPath":"clangd","capabilities":{},"trace":"off"}}
+---
+{"jsonrpc":"2.0","method":"textDocument/didOpen","params":{"textDocument":{"uri":"test:///simple.cpp","languageId":"cpp","version":1,"text":"void foo(); int main() { foo(); }\n"}}}
+---
+{"jsonrpc":"2.0","id":1,"method":"textDocument/symbolInfo","params":{"textDocument":{"uri":"test:///simple.cpp"},"position":{"line":0,"character":27}}}
+# CHECK:"containerName": null,
+# CHECK-NEXT:"id": "CA2EBE44A1D76D2A1547D47BC6D51EBF",
+# CHECK-NEXT:"name": "foo",
+# CHECK-NEXT:"usr": "c:@F@foo#"
+---
+{"jsonrpc":"2.0","method":"textDocument/didOpen","params":{"textDocument":{"uri":"test:///nested-decl.cpp","languageId":"cpp","version":1,"text":"namespace nnn { struct aaa {}; void foo() { aaa a; }; }"}}}
+---
+{"jsonrpc":"2.0","id":1,"method":"textDocument/symbolInfo","params":{"textDocument":{"uri":"test:///nested-decl.cpp"},"position":{"line":0,"character":46}}}
+# CHECK:"containerName": "nnn::",
+# CHECK-NEXT:"id": "20237FF18EB405D842456DC5D578426D",
+# CHECK-NEXT:"name": "aaa",
+# CHECK-NEXT:"usr": "c:@N@nnn@S@aaa"
+---
+{"jsonrpc":"2.0","method":"textDocument/didOpen","params":{"textDocument":{"uri":"test:///reference.cpp","languageId":"cpp","version":1,"text":"int value; void foo(int) {} void bar() { foo(value); }"}}}
+---
+{"jsonrpc":"2.0","id":1,"method":"textDocument/symbolInfo","params":{"textDocument":{"uri":"test:///reference.cpp"},"position":{"line":0,"character":48}}}
+# CHECK:"containerName": null,
+# CHECK-NEXT:"id": "844613FB2393C9D40A2AFF25D5D316A1",
+# CHECK-NEXT:"name": "value",
+# CHECK-NEXT:"usr": "c:@value"
+---
+{"jsonrpc":"2.0","method":"textDocument/didOpen","params":{"textDocument":{"uri":"test:///local.cpp","languageId":"cpp","version":1,"text":"void foo() { int aaa; int bbb = aaa; }"}}}
+---
+{"jsonrpc":"2.0","id":1,"method":"textDocument/symbolInfo","params":{"textDocument":{"uri":"test:///local.cpp"},"position":{"line":0,"character":33}}}
+# CHECK:"containerName": "foo",
+# CHECK-NEXT:"id": "C05589F2664B06F392C2C438568E55E0",
+# CHECK-NEXT:"name": "aaa",
+# CHECK-NEXT:"usr": "c:local.cpp@13@F@foo#@aaa"
+---
+{"jsonrpc":"2.0","method":"textDocument/didOpen","params":{"textDocument":{"uri":"test:///macro.cpp","languageId":"cpp","version":1,"text":"#define MACRO 5\nint i = MACRO;"}}}
+---
+{"jsonrpc":"2.0","id":1,"method":"textDocument/symbolInfo","params":{"textDocument":{"uri":"test:///macro.cpp"},"position":{"line":1,"character":11}}}
+# CHECK:"containerName": null,
+# CHECK-NEXT:"id": "29EB506CBDF1BA6D1B6EC203FF03B384",
+# CHECK-NEXT:"name": "MACRO",
+# CHECK-NEXT:"usr": "c:macro.cpp@24@macro@MACRO"
+---
+{"jsonrpc":"2.0","id":3,"method":"shutdown"}
+---
+{"jsonrpc":"2.0","method":"exit"}
Index: clangd/index/SymbolID.h
===
--- /dev/null
+++ clangd/index/SymbolID.h
@@ -0,0 +1,67 @@
+//===--- SymbolID.h --*- C++-*-===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===--===//
+
+#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANGD_INDEX_SYMBOLID_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANGD_INDEX_SYMBOLID_H
+
+#include "llvm/ADT/Hashing.h"
+#include "llvm/ADT/StringRef.h"
+#include "llvm/Support/Error.h"
+#include "llvm/Support/raw_ostream.h"
+#include
+#include
+
+namespace clang {
+namespace clangd {
+
+// The class identifies a particular C++ symbol (class, function, method, etc).
+//
+// As USRs (Unified Symbol Resolution) could be large, especially for functions
+// with long type arguments, SymbolID is using truncated SHA1(USR) values to
+// guarantee the uniqueness of symbols while using a relatively small amount of
+// memory (vs storing USRs directly).
+//
+// SymbolID can be used as key in the symbol indexes to lookup the symbol.
+class SymbolID {
+public
[PATCH] D54799: [clangd][WIP] textDocument/CursorInfo method
jkorous updated this revision to Diff 175230.
jkorous added a comment.
Removed empty line noise and fixed doxygen annotation.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D54799/new/
https://reviews.llvm.org/D54799
Files:
clangd/CMakeLists.txt
clangd/ClangdLSPServer.cpp
clangd/ClangdLSPServer.h
clangd/ClangdServer.cpp
clangd/ClangdServer.h
clangd/Protocol.cpp
clangd/Protocol.h
clangd/XRefs.cpp
clangd/XRefs.h
clangd/index/Index.cpp
clangd/index/Index.h
clangd/index/SymbolID.cpp
clangd/index/SymbolID.h
test/clangd/symbol-info.test
Index: test/clangd/symbol-info.test
===
--- /dev/null
+++ test/clangd/symbol-info.test
@@ -0,0 +1,46 @@
+# RUN: clangd -lit-test < %s | FileCheck %s
+{"jsonrpc":"2.0","id":0,"method":"initialize","params":{"processId":123,"rootPath":"clangd","capabilities":{},"trace":"off"}}
+---
+{"jsonrpc":"2.0","method":"textDocument/didOpen","params":{"textDocument":{"uri":"test:///simple.cpp","languageId":"cpp","version":1,"text":"void foo(); int main() { foo(); }\n"}}}
+---
+{"jsonrpc":"2.0","id":1,"method":"textDocument/symbolInfo","params":{"textDocument":{"uri":"test:///simple.cpp"},"position":{"line":0,"character":27}}}
+# CHECK:"containerName": null,
+# CHECK-NEXT:"id": "CA2EBE44A1D76D2A1547D47BC6D51EBF",
+# CHECK-NEXT:"name": "foo",
+# CHECK-NEXT:"usr": "c:@F@foo#"
+---
+{"jsonrpc":"2.0","method":"textDocument/didOpen","params":{"textDocument":{"uri":"test:///nested-decl.cpp","languageId":"cpp","version":1,"text":"namespace nnn { struct aaa {}; void foo() { aaa a; }; }"}}}
+---
+{"jsonrpc":"2.0","id":1,"method":"textDocument/symbolInfo","params":{"textDocument":{"uri":"test:///nested-decl.cpp"},"position":{"line":0,"character":46}}}
+# CHECK:"containerName": "nnn::",
+# CHECK-NEXT:"id": "20237FF18EB405D842456DC5D578426D",
+# CHECK-NEXT:"name": "aaa",
+# CHECK-NEXT:"usr": "c:@N@nnn@S@aaa"
+---
+{"jsonrpc":"2.0","method":"textDocument/didOpen","params":{"textDocument":{"uri":"test:///reference.cpp","languageId":"cpp","version":1,"text":"int value; void foo(int) {} void bar() { foo(value); }"}}}
+---
+{"jsonrpc":"2.0","id":1,"method":"textDocument/symbolInfo","params":{"textDocument":{"uri":"test:///reference.cpp"},"position":{"line":0,"character":48}}}
+# CHECK:"containerName": null,
+# CHECK-NEXT:"id": "844613FB2393C9D40A2AFF25D5D316A1",
+# CHECK-NEXT:"name": "value",
+# CHECK-NEXT:"usr": "c:@value"
+---
+{"jsonrpc":"2.0","method":"textDocument/didOpen","params":{"textDocument":{"uri":"test:///local.cpp","languageId":"cpp","version":1,"text":"void foo() { int aaa; int bbb = aaa; }"}}}
+---
+{"jsonrpc":"2.0","id":1,"method":"textDocument/symbolInfo","params":{"textDocument":{"uri":"test:///local.cpp"},"position":{"line":0,"character":33}}}
+# CHECK:"containerName": "foo",
+# CHECK-NEXT:"id": "C05589F2664B06F392C2C438568E55E0",
+# CHECK-NEXT:"name": "aaa",
+# CHECK-NEXT:"usr": "c:local.cpp@13@F@foo#@aaa"
+---
+{"jsonrpc":"2.0","method":"textDocument/didOpen","params":{"textDocument":{"uri":"test:///macro.cpp","languageId":"cpp","version":1,"text":"#define MACRO 5\nint i = MACRO;"}}}
+---
+{"jsonrpc":"2.0","id":1,"method":"textDocument/symbolInfo","params":{"textDocument":{"uri":"test:///macro.cpp"},"position":{"line":1,"character":11}}}
+# CHECK:"containerName": null,
+# CHECK-NEXT:"id": "29EB506CBDF1BA6D1B6EC203FF03B384",
+# CHECK-NEXT:"name": "MACRO",
+# CHECK-NEXT:"usr": "c:macro.cpp@24@macro@MACRO"
+---
+{"jsonrpc":"2.0","id":3,"method":"shutdown"}
+---
+{"jsonrpc":"2.0","method":"exit"}
Index: clangd/index/SymbolID.h
===
--- /dev/null
+++ clangd/index/SymbolID.h
@@ -0,0 +1,67 @@
+//===--- SymbolID.h --*- C++-*-===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===--===//
+
+#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANGD_INDEX_SYMBOLID_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANGD_INDEX_SYMBOLID_H
+
+#include "llvm/ADT/Hashing.h"
+#include "llvm/ADT/StringRef.h"
+#include "llvm/Support/Error.h"
+#include "llvm/Support/raw_ostream.h"
+#include
+#include
+
+namespace clang {
+namespace clangd {
+
+// The class identifies a particular C++ symbol (class, function, method, etc).
+//
+// As USRs (Unified Symbol Resolution) could be large, especially for functions
+// with long type arguments, SymbolID is using truncated SHA1(USR) values to
+// guarantee the uniqueness of symbols while using a relatively small amount of
+// memory (vs storing USRs directly).
+//
+// SymbolID can be used as key in the symbol indexes to lookup the symbol.
+class SymbolID {
+public:
+ SymbolID() = defau
