Relevant issue: https://github.com/clangd/clangd/issues/177
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
clang-tools-extra/clangd/Protocol.h | ||
---|---|---|
218 | Location is a common vocabulary type. (If this is just a shortcut taken while drafting this patch, please ignore me!) | |
clang-tools-extra/clangd/XRefs.cpp | ||
1508 | Being able to get all the containers in one ~constant-time index query is great! May still not want to have this on-by-default when the client doesn't advertise support, as it's a fixed nontrivial latency hit for remote index. Also we should skip this query when the set of IDs is empty. That can go here or (maybe better) in the remote index client. |
clang-tools-extra/clangd/XRefs.cpp | ||
---|---|---|
1508 |
Makes sense, we could either add a ClientCapability for this (Looks like we already have some capabilities related to extensions, so this wouldn't be a novelty), or it could just be a command line flag. Any preferences?
Agreed, probably more foolproof to let the index do this (otherwise the next person will forget about that as well probably) |
clang-tools-extra/clangd/unittests/XRefsTests.cpp | ||
---|---|---|
1941–1943 | @nridge See https://reviews.llvm.org/D137909 for this new syntax |
clang-tools-extra/clangd/Protocol.h | ||
---|---|---|
231 | This doesn't seem as clear as it could be: special how, additional to what? Maybe: /// Extends Locations returned by textDocument/references with extra info. /// This is a clangd extenison: LSP uses `Location`. | |
236 | "context" is vague, and is generally used in LSP to mean "details about how a request was triggered in the client". Unless we have a reason to keep this abstract, I'd use containerName as it's used elsewhere in LSP for this purpose. | |
244 | are these operators actually needed? especially operator< we usually get away without | |
clang-tools-extra/clangd/XRefs.cpp | ||
1306 | a few things to consider here:
| |
1309 | again for stringifying, I think we want the index/non-index cases to be the same, which suggests the logic to generate Scope+Name (i.e. clangd::printQualifiedName)+Signature and then concatenating them. Generating the signature is a bit complicated, so maybe leave it out (from both AST + index codepath) from initial patch? | |
1459 | To me, this doesn't seem like a sufficient reason to have irregular behavior for different refs. | |
1463 | nit: unconditional drop_back(2) seems brittle, consume_back instead? |
Since this is a protocol extension, you might want to add an end-to-end test: variant of clang-tools-extra/clangd/test/xrefs.test (xrefs-container.test? to avoid complicating all the existing tests).
And one this lands, one of us should update https://github.com/llvm/clangd-www/blob/main/extensions.md
I'll look into adding this, but I'm not familiar with with these kind of tests yet, so I'll might need some time to figure it out - I already uploaded an updated version of the patch anyway to get some more feedback in the meantime.
And one this lands, one of us should update https://github.com/llvm/clangd-www/blob/main/extensions.md
Yeah I could do that, I think I can even commit directly to that repo? Or should I open a review on github/phabricator for this anyway?
clang-tools-extra/clangd/Protocol.h | ||
---|---|---|
244 | Correct, they are indeed not needed as of now. | |
clang-tools-extra/clangd/XRefs.cpp | ||
1306 | Changed this to use SymbolCollector::getRefContainer, this should address most of the things you mentioned. Now AST and Index results should be consistent here | |
1309 | There's CodeCompletionString::getSignature, we could use that one probably? But anyway, the Signature field for index container results is actually empty right now for some reason (not sure if this a bug or a feature), so I removed Signature from index container results as well for now. Let's look into this in a follow-up patch. | |
1459 | Ok, changed it to Scope + Name here as well. If users complain about this, we can still change it later. |
clang-tools-extra/clangd/XRefs.cpp | ||
---|---|---|
1421 | Unlike the QueryIndex case, I think multiple references having the same container does not apply here (different overrides will be contained in different classes), but it's worth adding a comment to clarify that. | |
1482 | We can have multiple references with the same container (e.g. multiple references in the same function), so I think we need DenseMap<SymbolID, std::vector<size_t>> here. | |
1499 | For good measure, perhaps condition the container-related logic here on AddContext? | |
clang-tools-extra/clangd/unittests/XRefsTests.cpp | ||
2188 | Did you mean to add a call to B->func() as well here? | |
2237 | Did you mean to test the payload here? | |
2309 | nit: the payloads aren't actually used here | |
2358 | nit: we're not actually using the context | |
2375 | likewise |
Rebase, fix review comments
clang-tools-extra/clangd/XRefs.cpp | ||
---|---|---|
1482 | Good catch! Turns out this fixes the FIXME I added in FindReferences.RefsToBaseMethod | |
1499 | Good point, otherwise we're just wasting cycles and memory here if AddContext is false anyway | |
clang-tools-extra/clangd/unittests/XRefsTests.cpp | ||
2237 | Hmm I think I instead missed that this test does not use checkFindRefs. Removed the payload here, this path is already covered by other tests anyway. |
LGTM, and it looks like Sam's comments have been substantially addressed as well, so I'm going to go ahead and approve this, hope that's cool.
Thanks both of you for reviewing!
I added documentation to the website here: https://github.com/llvm/clangd-www/pull/77
Location is a common vocabulary type.
If we're going to add an extension that relates to a particular method, we should define a new type that's wire-compatible if that's at all feasible. Could be a subclass of Location or just a clone.
(If this is just a shortcut taken while drafting this patch, please ignore me!)