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. ---------------- nridge wrote: > 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. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D56370/new/ https://reviews.llvm.org/D56370 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
