[PATCH] D56370: [clangd] Add support for type hierarchy (super types only for now)

[Nathan Ridge via Phabricator via cfe-commits]

nridge marked 19 inline comments as done.
nridge added inline comments.


================
Comment at: clang-tools-extra/clangd/Protocol.cpp:830
+    return false;
+  if (auto *Resolve = Params.getAsObject()->get("resolve")) {
+    if (!fromJSON(*Resolve, R.resolve)) {
----------------
sammccall wrote:
> use ObjectMapper here? It's more idiomatic and avoids the issues below.
The reason I didn't use ObjectMapper is that I didn't know how to use it while also reusing the `fromJSON()` method for the base class, `TextDocumentPositionParams`.

I suppose I could just not reuse it (i.e. mention the fields of `TextDocumentPositionParams` in this function directly).


================
Comment at: clang-tools-extra/clangd/XRefs.cpp:885
+static Optional<TypeHierarchyItem>
+getTypeHierarchy(const CXXRecordDecl &CXXRD, ASTContext &ASTCtx, int Levels,
+                 TypeHierarchyDirection Direction) {
----------------
sammccall wrote:
> The scope and complexity of this function seems unneccesarily large:
>  - it's (in future?) capable of resolving in both directions
>  - it calls itself recursively, modifying TypeHierarchyDirection to control the search
>  - it handles levels, even though this optimization is only important for derived types
> 
> Can we restrict this to retrieving (all) recursive base types?
> i.e. `Optional<TypeHierarchyItem> getTypeAncestors(const CXXRecordDecl &CXXRD, ASTContext &Ctx)`
> Then later, subtype resolution can be the job of another function: `resolveTypeDescendants(TypeHierarchyItem& Item, int depth)`
> 
> That way the recursion of getTypeAncestors is simpler to understand, as it has much smaller scope. And code sharing between the two LSP calls is clearer: fetching type hierarchy is a call to `getTypeAncestors` and a call to `resolveTypeDescendants` (if direction is children or both, and resolve is > 0), and resolving a hierarchy is a call to `resolveTypeDescendants`.
If we remove "levels" here, should we introduce some kind of guard for infinite recursion, in case the user writes something like:

```
template <int N>
struct S : S<N + 1> {};

S<0> s;
```


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D56370/new/

https://reviews.llvm.org/D56370