kadircet added inline comments.
================
Comment at: clang-tools-extra/clangd/ClangdServer.cpp:365
Callback<tooling::Replacements> CB) {
- auto Action = [Sel](decltype(CB) CB, std::string File,
- std::string TweakID,
- Expected<InputsAndAST> InpAST) {
+ auto Action = [Sel](decltype(CB) CB, std::string File, std::string TweakID,
+ Expected<InputsAndAST> InpAST) {
----------------
Can you revert this change?
================
Comment at: clang-tools-extra/clangd/FindSymbols.h:16
#include "Protocol.h"
+#include "index/Index.h"
#include "llvm/ADT/StringRef.h"
----------------
this should rather be `clang/Index/IndexSymbol.h`. `index::SymbolKind` resides
in that header.
================
Comment at: clang-tools-extra/clangd/FindSymbols.h:21
+class ASTContext;
+class NamedDecl;
+
----------------
I don't think these two are necessary inside header file.
================
Comment at: clang-tools-extra/clangd/Protocol.cpp:871
+ O.map("children", I.children);
+ return true;
+}
----------------
Shouldn't we fail if some of the above fields(like `name`) fails to decode ?
================
Comment at: clang-tools-extra/clangd/XRefs.cpp:885
+static Optional<TypeHierarchyItem>
+getTypeHierarchy(const CXXRecordDecl &CXXRD, ASTContext &ASTCtx, int Levels,
+ TypeHierarchyDirection Direction) {
----------------
nridge wrote:
> 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;
> ```
clang should be limiting recursive template instantiations. Also since we are
just traversing the AST produced by clang, it should never be infinite, but
still a nice test case can you add one?
================
Comment at: clang-tools-extra/clangd/XRefs.cpp:918
+
+ return CXXRD;
+}
----------------
Nit: Get rid of CXXRD and return inside if/else branches.
================
Comment at: clang-tools-extra/clangd/XRefs.cpp:924
+
+ for (auto It = CXXRD->bases_begin(); It != CXXRD->bases_end(); It++) {
+ const CXXRecordDecl *ParentDecl = nullptr;
----------------
why not use a for-range loop ? (`CXXRD->bases()`)
================
Comment at: clang-tools-extra/unittests/clangd/XRefsTests.cpp:1562
+ const CXXRecordDecl *RD = findRecordTypeAt(AST, Pt);
+ EXPECT_EQ(nullptr, RD);
+ }
----------------
Can you put a TODO?
================
Comment at: clang-tools-extra/unittests/clangd/XRefsTests.cpp:1809
+ llvm::Optional<TypeHierarchyItem> Result =
+ getTypeHierarchy(AST, Pt, 10, TypeHierarchyDirection::Parents);
+ ASSERT_TRUE(bool(Result));
----------------
I suppose parent resolving should not depend on level, so what about setting it
to `0` instead of `10` so that test is consistent even after "child resolution"
implementation?
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
