[PATCH] D61497: [clangd] Introduce a structured hover response

This revision was automatically updated to reflect the committed changes. Closed by commit rL361803: [clangd] Introduce a structured hover response (authored by kadircet, committed by ). Herald added a project: LLVM. Herald added a subscriber: llvm-commits. Changed prior to commit:

[PATCH] D61497: [clangd] Introduce a structured hover response

kadircet updated this revision to Diff 201628. kadircet added a comment. - clang-format Definition Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D61497/new/ https://reviews.llvm.org/D61497 Files: clang-tools-extra/clangd/ClangdLSPServer.cpp

[PATCH] D61497: [clangd] Introduce a structured hover response

kadircet updated this revision to Diff 201543. kadircet marked 7 inline comments as done. kadircet added a comment. - Address comments Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D61497/new/ https://reviews.llvm.org/D61497 Files:

[PATCH] D61497: [clangd] Introduce a structured hover response

sammccall added a comment. Still LG I forgot to mention - it would be nice to clang-format the definition, what do you think? Comment at: clang-tools-extra/clangd/unittests/XRefsTests.cpp:592 +)cpp", + {/*NamespaceScope=*/std::string("ns1::ns2"), +

[PATCH] D61497: [clangd] Introduce a structured hover response

kadircet updated this revision to Diff 201536. kadircet marked 10 inline comments as done. kadircet added a comment. - Address comments Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D61497/new/ https://reviews.llvm.org/D61497 Files:

[PATCH] D61497: [clangd] Introduce a structured hover response

kadircet marked 10 inline comments as done. kadircet added inline comments. Comment at: clang-tools-extra/clangd/unittests/XRefsTests.cpp:567 +const HoverInfo Expected; + } // namespace + Cases[] = { sammccall wrote: > namespace?! > > might be a

[PATCH] D61497: [clangd] Introduce a structured hover response

sammccall accepted this revision. sammccall added a comment. This revision is now accepted and ready to land. Thanks, I think you're right about all of this. Implementation looks OK too. Please add TODOs for the cases where we're punting to later vs deciding not to fix. In D61497#1512003

[PATCH] D61497: [clangd] Introduce a structured hover response

kadircet added inline comments. Comment at: clang-tools-extra/clangd/unittests/XRefsTests.cpp:940 )cpp", - "class std::initializer_list", + "template<> class initializer_list {}", }, kadircet wrote: > sammccall wrote: > > hmm,

[PATCH] D61497: [clangd] Introduce a structured hover response

kadircet updated this revision to Diff 200732. kadircet marked 5 inline comments as done. kadircet added a comment. - Added tests, and went over implementation. I am looking for input on: - Behavior on lambdas, currently it just keeps the old behaviour. I believe they should look more like

[PATCH] D61497: [clangd] Introduce a structured hover response

kadircet updated this revision to Diff 199257. kadircet marked 2 inline comments as done. kadircet added a comment. - Address comments Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D61497/new/ https://reviews.llvm.org/D61497 Files:

[PATCH] D61497: [clangd] Introduce a structured hover response

kadircet marked 16 inline comments as done. kadircet added a comment. Thanks for also taking a look at the implementation, it is not complete yet. I am rather waiting for a green light on struct itself, so that I can write tests. Comment at:

[PATCH] D61497: [clangd] Introduce a structured hover response

sammccall added a comment. Main comment is that I think the code is doing too much work to exactly reproduce the current output, and include as much information as possible. Minimizing the diff is good all else equal, but one of the goals here is to have richer hovercards that are more

[PATCH] D61497: [clangd] Introduce a structured hover response

sammccall added a comment. comments on interface again, will take another pass at implementation Comment at: clang-tools-extra/clangd/XRefs.h:65 + /// Fully qualiffied name for the scope containing the Sym. + std::string Scope; + std::string ParentScope;

[PATCH] D61497: [clangd] Introduce a structured hover response

kadircet marked 6 inline comments as done. kadircet added inline comments. Comment at: clang-tools-extra/clangd/XRefs.cpp:539 -/// Generate a \p Hover object given the declaration \p D. -static Hover getHoverContents(const Decl *D) { - Hover H; - llvm::Optional NamedScope =

[PATCH] D61497: [clangd] Introduce a structured hover response

kadircet updated this revision to Diff 198630. kadircet marked 23 inline comments as done. kadircet added a comment. - Address comments - Make little modifications to existing tests Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D61497/new/

[PATCH] D61497: [clangd] Introduce a structured hover response

kadircet added inline comments. Comment at: clang-tools-extra/clangd/XRefs.cpp:574 - // We want to include the template in the Hover. - if (TemplateDecl *TD = D->getDescribedTemplate()) -D = TD; +llvm::raw_ostream <<(llvm::raw_ostream , +

[PATCH] D61497: [clangd] Introduce a structured hover response

sammccall added inline comments. Comment at: clang-tools-extra/clangd/XRefs.h:53 +struct HoverInfo { + using Type = std::string; + struct Param { ilya-biryukov wrote: > NIT: maybe go with `std::string Type` at a use-site instead? > The scope of the name is

[PATCH] D61497: [clangd] Introduce a structured hover response

sammccall added a comment. (I only got about halfway through the implementation - it's missing a lot of comments I think ;-)) Comment at: clang-tools-extra/clangd/XRefs.cpp:539 -/// Generate a \p Hover object given the declaration \p D. -static Hover getHoverContents(const

[PATCH] D61497: [clangd] Introduce a structured hover response

ilya-biryukov added inline comments. Comment at: clang-tools-extra/clangd/XRefs.h:53 +struct HoverInfo { + using Type = std::string; + struct Param { NIT: maybe go with `std::string Type` at a use-site instead? The scope of the name is large enough to make

[PATCH] D61497: [clangd] Introduce a structured hover response

kadircet updated this revision to Diff 198457. kadircet marked 2 inline comments as done. kadircet added a comment. I will start adding test cases(and most likely change the way I populate the struct at some places) if everyone is happy with current representation for `HoverInfo`. Repository:

[PATCH] D61497: [clangd] Introduce a structured hover response

kadircet added inline comments. Comment at: clang-tools-extra/clangd/XRefs.h:57 +// template class. +Type T; +std::string Name; ilya-biryukov wrote: > Same about type template parameters. > Probably just holds `class` or `typename`? Exactly, adding

[PATCH] D61497: [clangd] Introduce a structured hover response

ilya-biryukov added inline comments. Comment at: clang-tools-extra/clangd/XRefs.h:57 +// template class. +Type T; +std::string Name; Same about type template parameters. Probably just holds `class` or `typename`? Repository: rG LLVM Github

[PATCH] D61497: [clangd] Introduce a structured hover response

ilya-biryukov added inline comments. Comment at: clang-tools-extra/clangd/XRefs.h:73 + llvm::Optional> Parameters; + llvm::Optional> TemplateParameters; + kadircet wrote: > ilya-biryukov wrote: > > What does `Type` mean for non-type and template template

[PATCH] D61497: [clangd] Introduce a structured hover response

kadircet updated this revision to Diff 198420. kadircet marked an inline comment as done. kadircet added a comment. - Add comments for non-type params Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D61497/new/ https://reviews.llvm.org/D61497 Files:

[PATCH] D61497: [clangd] Introduce a structured hover response

kadircet marked 2 inline comments as done. kadircet added inline comments. Comment at: clang-tools-extra/clangd/XRefs.h:73 + llvm::Optional> Parameters; + llvm::Optional> TemplateParameters; + ilya-biryukov wrote: > What does `Type` mean for non-type and

[PATCH] D61497: [clangd] Introduce a structured hover response

ilya-biryukov added a comment. Was passing by, just a small clarifying question... Comment at: clang-tools-extra/clangd/XRefs.h:73 + llvm::Optional> Parameters; + llvm::Optional> TemplateParameters; + What does `Type` mean for non-type and template template

[PATCH] D61497: [clangd] Introduce a structured hover response

kadircet added inline comments. Comment at: clang-tools-extra/clangd/XRefs.h:52 +struct HoverInfo { + LocatedSymbol Sym; + /// Name of the context containing the symbol. sammccall wrote: > I'm not sure about reuse of LocatedSymbol - do we want to commit to

[PATCH] D61497: [clangd] Introduce a structured hover response

kadircet updated this revision to Diff 198295. kadircet marked 8 inline comments as done. kadircet added a comment. - Address comments Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D61497/new/ https://reviews.llvm.org/D61497 Files:

[PATCH] D61497: [clangd] Introduce a structured hover response

sammccall added a comment. Layering and such looks good. This should compose well with D58547 Comment at: clang-tools-extra/clangd/XRefs.h:52 +struct HoverInfo { + LocatedSymbol Sym; + /// Name of the context containing the symbol.

[PATCH] D61497: [clangd] Introduce a structured hover response

kadircet planned changes to this revision. kadircet added a comment. This is still WIP, just looking for feedbacks on `HoverInfo` struct and overall layering Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D61497/new/ https://reviews.llvm.org/D61497

[PATCH] D61497: [clangd] Introduce a structured hover response

kadircet created this revision. kadircet added a reviewer: sammccall. Herald added subscribers: cfe-commits, arphaman, jkorous, MaskRay, ilya-biryukov. Herald added a project: clang. Change ClangdServer layer to output a structured response for Hover, which can be rendered by client according to