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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
