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

This revision was automatically updated to reflect the committed changes. Closed by commit rL356445: [clangd] Add support for type hierarchy (super types only for now) (authored by kadircet, committed by ). Herald added a project: LLVM. Herald added a subscriber: llvm-commits. Changed prior to

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

nridge added a comment. Great, thanks for the reviews! Could you commit the patch as well? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D56370/new/ https://reviews.llvm.org/D56370 ___ cfe-commits

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

kadircet accepted this revision. kadircet added a comment. This revision is now accepted and ready to land. LGTM, thanks Nathan! Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D56370/new/ https://reviews.llvm.org/D56370

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

ilya-biryukov added inline comments. Comment at: clang-tools-extra/clangd/XRefs.h:62 +/// Find the record type references at \p Pos. +const CXXRecordDecl *findRecordTypeAt(ParsedAST , Position Pos); + nridge wrote: > ilya-biryukov wrote: > > This method looks

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

nridge updated this revision to Diff 190755. nridge marked an inline comment as done. nridge added a comment. Address latest review comments Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D56370/new/ https://reviews.llvm.org/D56370 Files:

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

nridge marked 7 inline comments as done. nridge added inline comments. Comment at: clang-tools-extra/clangd/XRefs.h:62 +/// Find the record type references at \p Pos. +const CXXRecordDecl *findRecordTypeAt(ParsedAST , Position Pos); + ilya-biryukov wrote: > This

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

ilya-biryukov added a comment. A few small NITs Comment at: clang-tools-extra/clangd/ClangdServer.h:188 + /// Get information about type hierarchy for a given position. + void findTypeHierarchy(PathRef File, Position Pos, int Resolve, +

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

ioeric added a comment. just a few drive-by comments ;) Comment at: clang-tools-extra/clangd/FindSymbols.h:28 +// https://github.com/Microsoft/language-server-protocol/issues/344 +SymbolKind indexSymbolKindToSymbolKind(index::SymbolKind Kind); + This could

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

kadircet added a comment. In D56370#1427075 , @nridge wrote: > Ok, I filed a Theia issue > about it for now. Thanks! In D56370#1427076 , @nridge wrote: > Are

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

nridge updated this revision to Diff 190375. nridge added a comment. Address remaining review comments (I figure out how to write a lit test) Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D56370/new/ https://reviews.llvm.org/D56370 Files:

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

nridge added a comment. In D56370#1426234 , @kadircet wrote: > Unfortunately we usually test LSP layer with lit tests, and since we don't > have any lit tests for this use-case it wasn't caught. Could you add a lit > test for overall use case? You can

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

nridge added a comment. > In D56370#1424180 , @nridge wrote: > >> Unfortunately, there is a further problem: the Theia client-side >> implementation of type hierarchy has recently merged, and their code has >> changed so that they do require

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

kadircet added inline comments. Comment at: clang-tools-extra/unittests/clangd/XRefsTests.cpp:1496 +TEST(FindRecordTypeAt, TypeOrVariable) { + Annotations Source(R"cpp( Sorry for not pointing this out before, but it would be great if you could move related

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

kadircet marked an inline comment as done. kadircet added a comment. In D56370#1424177 , @nridge wrote: > Fix a (somewhat amusing) typo where I wrote '0' instead of 'O' in a > fromJSON() implementation > > (Does the fact that this didn't cause any test

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

nridge added a comment. Unfortunately, there is a further problem: the Theia client-side implementation of type hierarchy has recently merged, and their code has changed so that they do require `typeHierarchy/resolve` to be supported. They ask for 1 level in the initial request, ignore any

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

nridge updated this revision to Diff 190041. nridge added a comment. Fix a (somewhat amusing) typo where I wrote '0' instead of 'O' in a fromJSON() implementation (Does the fact that this didn't cause any test failures suggest that the fromJSON() functions aren't exercised by any tests?)

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

nridge marked an inline comment as done. nridge added a comment. The updated patch addresses the infinite recursion issue by bailing on dependent bases for now, as Sam suggested. I will implement the more comprehensive suggested fix ("bail out once we see instantiations of the same template

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

nridge updated this revision to Diff 190040. nridge added a comment. Address the infinite recursion issue Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D56370/new/ https://reviews.llvm.org/D56370 Files:

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

kadircet added inline comments. Comment at: clang-tools-extra/clangd/ClangdServer.cpp:365 Callback CB) { - auto Action = [Sel](decltype(CB) CB, std::string File, -std::string TweakID, -

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

nridge marked an inline comment as done. nridge added inline comments. Comment at: clang-tools-extra/clangd/XRefs.cpp:885 +static Optional +getTypeHierarchy(const CXXRecordDecl , ASTContext , int Levels, + TypeHierarchyDirection Direction) {

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

nridge added inline comments. Comment at: clang-tools-extra/clangd/ClangdServer.cpp:365 Callback CB) { - auto Action = [Sel](decltype(CB) CB, std::string File, -std::string TweakID, -Expected

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

nridge updated this revision to Diff 189444. nridge marked 10 inline comments as done. nridge added a comment. Address most of the latest review comments. The infinite recursion issue remains to be fixed. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

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

sammccall added a comment. Thanks for picking this up... just wanted to leave one last comment as I'd been thinking about the recursive template case too. Comment at: clang-tools-extra/clangd/XRefs.cpp:885 +static Optional +getTypeHierarchy(const CXXRecordDecl , ASTContext ,

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

kadircet added inline comments. Comment at: clang-tools-extra/clangd/ClangdServer.cpp:365 Callback CB) { - auto Action = [Sel](decltype(CB) CB, std::string File, -std::string TweakID, -

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

nridge updated this revision to Diff 189114. nridge added a comment. Address a couple of outstanding TODOs Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D56370/new/ https://reviews.llvm.org/D56370 Files:

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

nridge updated this revision to Diff 189092. nridge marked 2 inline comments as done. nridge added a comment. Address latest review comments Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D56370/new/ https://reviews.llvm.org/D56370 Files:

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

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

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

sammccall added a comment. Hi Nathan, Really sorry about the delay here. I'm actually out on leave at the moment, and didn't get things wrapped up before I went. (I'm 1 week into 4 week parental leave, and got sick the week I was supposed to be handing things off). @kadircet, can you finish

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

nridge updated this revision to Diff 189065. nridge added a comment. Rebased. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D56370/new/ https://reviews.llvm.org/D56370 Files: clang-tools-extra/clangd/ClangdLSPServer.cpp

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

nridge added a comment. I think this is ready to continue to be reviewed :) Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D56370/new/ https://reviews.llvm.org/D56370 ___ cfe-commits mailing list

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

nridge updated this revision to Diff 186947. nridge added a comment. Fix a test failure Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D56370/new/ https://reviews.llvm.org/D56370 Files: clang-tools-extra/clangd/ClangdLSPServer.cpp

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

nridge marked an inline comment as done. nridge added inline comments. Comment at: clang-tools-extra/unittests/clangd/XRefsTests.cpp:1604 + dyn_cast((AST, "Parent"))->getTemplatedDecl(); + // TODO: This fails because findDecl() doesn't support template-ids + // const

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

nridge updated this revision to Diff 186944. nridge added a comment. - Rework tests to test the lower-level functions like typeAncestors() - Remove support for typeHierarchy/resolve Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D56370/new/

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

sammccall added inline comments. Comment at: clang-tools-extra/clangd/XRefs.cpp:842 + + StringRef Filename = SM.getFilename(BeginLoc); + std::string FileURI = toURI(SM, Filename, {}); sammccall wrote: > This is more conversions than necessary. > I think we

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

sammccall added inline comments. Comment at: clang-tools-extra/clangd/ClangdServer.cpp:543 + return CB(InpAST.takeError()); +CB(clangd::getTypeHierarchy(InpAST->AST, Item.range.start, Resolve, +Direction)); nridge wrote:

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

nridge marked 2 inline comments as done. nridge added inline comments. Comment at: clang-tools-extra/clangd/ClangdServer.cpp:543 + return CB(InpAST.takeError()); +CB(clangd::getTypeHierarchy(InpAST->AST, Item.range.start, Resolve, +

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

sammccall added inline comments. Comment at: clang-tools-extra/clangd/ClangdServer.cpp:543 + return CB(InpAST.takeError()); +CB(clangd::getTypeHierarchy(InpAST->AST, Item.range.start, Resolve, +Direction)); nridge wrote:

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

nridge marked an inline comment as done. nridge added inline comments. Herald added a subscriber: jdoerfert. Comment at: clang-tools-extra/clangd/ClangdServer.cpp:543 + return CB(InpAST.takeError()); +CB(clangd::getTypeHierarchy(InpAST->AST, Item.range.start, Resolve, +

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

kadircet added inline comments. Comment at: clang-tools-extra/clangd/XRefs.cpp:925 +CXXRD = VD->getType().getTypePtr()->getAsCXXRecordDecl(); + } else if (const CXXMethodDecl *Method = dyn_cast(D)) { +// If this is a method, use the type of the class.

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

(Sorry, hit enter too soon and truncated one of the comments) On Mon, Feb 11, 2019 at 10:32 AM Sam McCall via Phabricator < revi...@reviews.llvm.org> wrote: > sammccall added a comment. > > In D56370#1391924 , @nridge > wrote: > > > As far as reworking

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

sammccall added a comment. In D56370#1391924 , @nridge wrote: > As far as reworking the tests to use these functions, I've thought about that > a bit: > > - These functions return AST nodes. It's not clear to me how I would come up > with "expected" AST

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

nridge added a comment. In D56370#1390359 , @sammccall wrote: > - in 'XRefs.h', I think the API to introduce is `findTypeAt(Position) -> > Decl*` + `typeAncestors(Decl*)` and later `typeDescendants(Decl*)`, rather > than a single more complex

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

nridge updated this revision to Diff 186139. nridge added a comment. Introduce and use findRecordTypeAt() and typeAncestors() helpers Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D56370/new/ https://reviews.llvm.org/D56370 Files:

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

nridge added a comment. In D56370#1390359 , @sammccall wrote: > So on a concrete but still high-level: > > - I don't think we should implement the `resolve` operation, or resolution > bounds, at this point - this patch doesn't do anything slow. Returning

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

nridge updated this revision to Diff 186137. nridge marked 4 inline comments as done. nridge added a comment. Address Kadir's review comments Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D56370/new/ https://reviews.llvm.org/D56370 Files:

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

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 resolve; + kadircet wrote: > why not just use an

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

nridge added a comment. In D56370#1391904 , @nridge wrote: > Add tests for scenarios involving class template specializations > > Also improve support for dependent bases (This update is unrelated to the review comments, it's just improvements I had in

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

nridge updated this revision to Diff 186136. nridge added a comment. Add tests for scenarios involving class template specializations Also improve support for dependent bases Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D56370/new/

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

sammccall added a comment. Updated reviewers line to reflect (I think) current reality so this doesn't get lost, but feel free to reassign as you'd prefer. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D56370/new/ https://reviews.llvm.org/D56370

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

sammccall added a comment. In D56370#1390283 , @kadircet wrote: > Implementation LG, but I am not sure about adding a functionality on a > proposal that might change. WDYT @sammccall ? We discussed this a bit on the thread, I think we should follow the

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

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

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

nridge marked 3 inline comments as done. nridge added inline comments. Comment at: clang-tools-extra/clangd/XRefs.cpp:820 +// TODO: Reduce duplication between this function and declToSym(). +static llvm::Optional I am open to feedback on whether we want to

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

nridge updated this revision to Diff 184985. nridge added a comment. remove unrelated file Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D56370/new/ https://reviews.llvm.org/D56370 Files: clang-tools-extra/clangd/ClangdLSPServer.cpp

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

nridge updated this revision to Diff 184984. nridge edited the summary of this revision. nridge added a comment. Herald added a project: clang. Herald added a subscriber: cfe-commits. This completes the implementation of the revised spec (for supertypes only) Repository: rG LLVM Github