[PATCH] D56370: [clangd] Add support for type hierarchy (super types only for now)
Sam McCall via Phabricator via cfe-commits
cfe-commits at lists.llvm.org
Fri Feb 8 04:26:27 PST 2019
sammccall added a comment.
In D56370#1390283 <https://reviews.llvm.org/D56370#1390283>, @kadircet wrote:
> Implementation LG, but I am not sure about adding a functionality on a proposal that might change. WDYT @sammccall ?
We discussed this a bit on the thread, I think we should follow the proposed spec, but with some useful hedges:
- only implement enough bits to be useful
- I'm not sure the internal APIs/implementation should follow the "shape" of the spec so deeply.
So on a concrete but still high-level:
- I don't think we should implement the `resolve` operation, or resolution bounds, at this point - this patch doesn't do anything slow. Returning complete ancestors and never returning any children seems simplest.
- in 'XRefs.h', I think the API to introduce is `findTypeAt(Position) -> Decl*` + `typeAncestors(Decl*)` and later `typeDescendants(Decl*)`, rather than a single more complex `typeHierarchy` call. These two operations have little in common implementation-wise, and it's easy to imagine editors preferring to expose them separately. In clangdserver of course we need to expose a single operation because of transactionality. The stitching together could go in clangdserver, or a higher-level function exposed by xrefs - but I think the separate functions are what we should be unit-testing.
Comment at: clang-tools-extra/unittests/clangd/Matchers.h:130
+// Implements the HasValue(m) matcher for matching an Optional whose
+// value matches matcher m.
> Should I split this out into a separate patch? It's used by the tests being added for this functionality.
This is nice! I think it should probably go in llvm/Testing/Support/..., but it's OK here.
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
More information about the cfe-commits