[PATCH] D56370: [clangd] Add support for type hierarchy (super types only for now)
Nathan Ridge via Phabricator via cfe-commits
cfe-commits at lists.llvm.org
Sat Feb 9 17:29:20 PST 2019
nridge added a comment.
In D56370#1390359 <https://reviews.llvm.org/D56370#1390359>, @sammccall wrote:
> - 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.
In the latest update I introduced these helpers. I made the first one `findRecordTypeAt()` instead of `findTypeAt()`, since `findTypeAt()` suggests it also works for built-in types like `int`, but I don't think we can get a `Decl*` for `int`.
As far as reworking the tests to use these functions, I've thought about that a bit:
- These functions return AST nodes. It's not clear to me how I would come up with "expected" AST nodes to test the return values against.
- Even if we find a way to get "expected" AST nodes, we would be losing test coverage of functions like `declToTypeHierarchyItem()` (though I suppose we could write separate tests for that).
- We could instead test against the //properties// of the AST nodes, such as their names and source ranges, but then the test code to query those properties would basically be duplicating code in `declToTypeHierarchyItem()`. It would seem to make more sense to just test the resulting `TypeHierarchyItem` instead (as the tests are doing now).
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
More information about the cfe-commits