kadircet added a subscriber: sammccall. kadircet added a comment. Implementation LG, but I am not sure about adding a functionality on a proposal that might change. WDYT @sammccall ?
================ Comment at: clang-tools-extra/clangd/FindSymbols.cpp:16 #include "SourceCode.h" +#include "XRefs.h" #include "index/Index.h" ---------------- it seems like there is no change in file requiring this ================ Comment at: clang-tools-extra/clangd/FindSymbols.cpp:218 } + return SI; ---------------- can you revert this one as well ? ================ Comment at: clang-tools-extra/clangd/Protocol.cpp:817 +bool fromJSON(const llvm::json::Value &E, TypeHierarchyDirection &Out) { + if (auto T = E.getAsInteger()) { + if (*T < static_cast<int>(TypeHierarchyDirection::Children) || ---------------- nit: get rid of nesting by ``` auto T = E.getas..; if(!T) return false; if(*T not in rage) return false; Out = cast(*T); return true; ``` ================ Comment at: clang-tools-extra/clangd/Protocol.h:1026 + /// When not defined, it is treated as `0`. + llvm::Optional<int> resolve; + ---------------- why not just use an int then ? ================ Comment at: clang-tools-extra/clangd/Protocol.h:1046 + /// It is `false` by default. + llvm::Optional<bool> deprecated; + ---------------- again we can just store a bool, and only serialize if it is true ================ Comment at: clang-tools-extra/clangd/XRefs.cpp:876 + Optional<TypeHierarchyItem> Result = declToTypeHierarchyItem(ASTCtx, CXXRD); + if (Result && Levels > 0) { + if (Direction == TypeHierarchyDirection::Parents || ---------------- again reduce nesting ================ 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)); ---------------- 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 ? ================ Comment at: clang-tools-extra/clangd/XRefs.cpp:917 + if (Symbols.Decls.empty()) + return {}; + ---------------- llvm::None ================ 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. ---------------- what about member fields ? ================ Comment at: clang-tools-extra/clangd/XRefs.cpp:932 + + if (CXXRD) { + int ResolveLevels = Resolve.getValueOr(0); ---------------- reduce nesting ================ Comment at: clang-tools-extra/clangd/XRefs.cpp:935 + TypeHierarchyDirection ResolveDirection = + Direction.getValueOr(TypeHierarchyDirection::Parents); + return getTypeHierarchy(*CXXRD, ASTCtx, ResolveLevels, ResolveDirection); ---------------- Is this mentioned in proposal? ================ Comment at: clang-tools-extra/clangd/XRefs.cpp:939 + + return {}; +} ---------------- llvm::None ================ Comment at: clang-tools-extra/unittests/clangd/XRefsTests.cpp:1355 + Annotations Source(R"cpp( +struct $ParentDef[[Parent]] +{ ---------------- can you format the code? ================ Comment at: clang-tools-extra/unittests/clangd/XRefsTests.cpp:1387 + + for (auto Pt : {"p1", "p2", "p3", "p4"}) { + llvm::Optional<TypeHierarchyItem> Result = getTypeHierarchy( ---------------- You can use `Source.points()` and use `Pt` directly instead of doing `Source.point(pt)`. Same for other tests. 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