nridge marked 19 inline comments as done. nridge added a comment. Thank you for the reviews!
================ Comment at: clang-tools-extra/clangd/Protocol.h:1026 + /// When not defined, it is treated as `0`. + llvm::Optional<int> resolve; + ---------------- kadircet wrote: > why not just use an int then ? I was making the data structure correspond closely to the protocol, where the field is optional. But we can handle this in the serialization logic, yes. ================ Comment at: clang-tools-extra/clangd/XRefs.cpp:891 + if (Optional<TypeHierarchyItem> ParentSym = + getTypeHierarchy(*ParentDecl, ASTCtx, Levels - 1, Direction)) { + Result->parents->emplace_back(std::move(*ParentSym)); ---------------- kadircet wrote: > A problem that might occur when you add children traversal: > do we really want to include current decl in children of parent when we have > `BOTH` as direction? If so, maybe we should rather cache the results? Maybe > leave a todo/fixme ? Actually, this is a bug: if the top-level call uses `Both`, the recursive calls should still just use `Parents` and `Children` (i.e. we never want children of parents or parents of children). Thanks for spotting that! ================ Comment at: clang-tools-extra/clangd/XRefs.cpp:925 + CXXRD = VD->getType().getTypePtr()->getAsCXXRecordDecl(); + } else if (const CXXMethodDecl *Method = dyn_cast<CXXMethodDecl>(D)) { + // If this is a method, use the type of the class. ---------------- kadircet wrote: > what about member fields ? It's not clear what the desired semantics would be for a member field: get the type hierarchy of the enclosing class type, or the type hierarchy of the field's type? ================ Comment at: clang-tools-extra/clangd/XRefs.cpp:935 + TypeHierarchyDirection ResolveDirection = + Direction.getValueOr(TypeHierarchyDirection::Parents); + return getTypeHierarchy(*CXXRD, ASTCtx, ResolveLevels, ResolveDirection); ---------------- kadircet wrote: > Is this mentioned in proposal? It's not specified; I left a [comment on the proposal](https://github.com/Microsoft/vscode-languageserver-node/pull/426/commits/876d7fe224e17d7c08258a6da21d11a2df9c1d0d#r252942213) asking about it. ================ Comment at: clang-tools-extra/unittests/clangd/Matchers.h:130 +// Implements the HasValue(m) matcher for matching an Optional whose +// value matches matcher m. ---------------- sammccall wrote: > 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. I'll leave it here for now. We can move it in a follow-up patch. 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