Details
Diff Detail
- Repository
- rCTE Clang Tools Extra
Event Timeline
clangd/XRefs.cpp | ||
---|---|---|
663 | Are we going to support the IncludeDeclaration option? I'm trying to figure out a better name for it, can't get what should it do by looking at the code at the moment :-) |
clangd/XRefs.cpp | ||
---|---|---|
663 | I think so. It is a term defined in LSP (although vscode always sets it true). I think it aligns with the clang::index::SymbolRole::Declaration. |
This change LG, but I would not commit it before we have an actual implementation.
As soon as we have the references function in ClangdUnit.cpp implemented, the merge of this change should be trivial.
Is there any value in committing empty stubs before an actual implementation is ready?
Actually, I'd prefer to get it submitted before we have the actual implementation -- because it would make use easier to experience xrefs features in LSP clients when adding xrefs actual implemenation (without patching the whole patch locally).
Is there any concern about it? Since we disable it by default, it would not affect any users.
Having unimplemented stubs in the codebase creates some confusion, but that's my personal opinion. Not terribly opposed to that if others feel it's the right way to go.
For experiments, we could simply patch this in locally without committing upstream. With git it's incredibly easy.
Agreed with Ilya. I'd probably also make this depend on the ongoing implementation, as exposing LSP endpoints without proper implementation might be confusing to clangd users who only look at the the LSP endpoints. Users need to dig two levels of abstraction to find out that it's not implemented.
clangd/ClangdServer.h | ||
---|---|---|
161 | I think the C++ API can return SymbolOccurrence in the callback, which would allow C++ API users to get richer information about the occurrences e.g. kind, relationship, code snippet. | |
161 | nit: s/includeDeclaration/IncludeDeclaration/ |
Updated based on findReferences implementation which has now landed.
Removed ReferenceContext support for now, implementation has no options.
references->findReferences in ClangdServer (more consistent with other methods)
Added lit test for this feature.
Set this feature to true in LSP capabilities.
Made initialize-params-invalid less brittle instead of updating it.
PTAL
clangd/ClangdServer.h | ||
---|---|---|
161 | I hadn't seen this comment, I agree, but the implementation that landed just returns Locations. We can extend the C++ API but it doesn't belong in this patch. | |
clangd/XRefs.cpp | ||
663 | It's not supported in the underlying implementation today, partly because we weren't sure FWIW I think it should control whether the roles Declaration *and* Definition are included, because I guess it's meant to distinguish between usages that introduce a name vs usages that reference an existing name. For now, we don't support any options and don't parse them out of the LSP. |
Thanks, LGTM, ship it!
clangd/Protocol.h | ||
---|---|---|
881 | Maybe add a comment saying we don't support includeDeclaration? |
I think the C++ API can return SymbolOccurrence in the callback, which would allow C++ API users to get richer information about the occurrences e.g. kind, relationship, code snippet.