Page MenuHomePhabricator

[clangd] Represent Hover result using FormattedString
ClosedPublic

Authored by ilya-biryukov on May 6 2019, 10:48 AM.

Diff Detail

Repository
rL LLVM

Event Timeline

ilya-biryukov created this revision.May 6 2019, 10:48 AM
Herald added a project: Restricted Project. · View Herald TranscriptMay 6 2019, 10:48 AM

I think it makes more sense to expose semantical information in HoverInfo(like Name, Scope, Definition, etc), rather than formatted strings, and let this be serialized by the users of ClangdServer (as in D61497). This is what I'll perform with that patch anyway, since it aims to provide consumers of ClangdServer with sementical information. So it is up to you whether to change the layering or keep it as it is in this patch.

clang-tools-extra/clangd/ClangdLSPServer.cpp
853 ↗(On Diff #198307)

Why not move R.contents.kind and R.range assignment out of the switch statement. and perform return after the switch. That way you can get rid of the llvm_unreachable(we would still get warnings for that switch-statement if someone adds a new MarkupKind but doesn't update that statement.

And hopefully we should fallback to PlainText if editor doesn't support any markupkinds known to us.

clang-tools-extra/clangd/Protocol.cpp
707 ↗(On Diff #198307)

Maybe also vlog/elog the unknown kind? So that we can easily catch new additions to spec.

clang-tools-extra/clangd/Protocol.h
359 ↗(On Diff #198307)

This is mentioned as PlainText in the specs

I think it makes more sense to expose semantical information in HoverInfo(like Name, Scope, Definition, etc), rather than formatted strings, and let this be serialized by the users of ClangdServer (as in D61497). This is what I'll perform with that patch anyway, since it aims to provide consumers of ClangdServer with sementical information. So it is up to you whether to change the layering or keep it as it is in this patch.

Having structured information instead of FormattedString looks good, this change mostly aims to add markdown support for resulting hover when the LSP clients have support for it.
I'm happy to restructure the HoverInfo to your liking if the current one seems problematic for any reason.

ilya-biryukov marked 3 inline comments as done.May 7 2019, 1:34 AM
ilya-biryukov added inline comments.
clang-tools-extra/clangd/ClangdLSPServer.cpp
853 ↗(On Diff #198307)

Done, we do want llvm_unreachable at the end, just in case a new MarkupKind gets added.

And hopefully we should fallback to PlainText if editor doesn't support any markupkinds known to us.

This is handled by the code parsing initialization options and it actually falls back to plaintext.

  • Move range and kind assignment out of the loop.
  • Log error on failed deserialization of markup kind.
  • s/Plaintext/PlainText

I'm happy to restructure the HoverInfo to your liking if the current one seems problematic for any reason.

It will just cause more code move on my side, no problems apart from that

clang-tools-extra/clangd/Protocol.cpp
707 ↗(On Diff #198307)

I was referring to the else case down below :D

ilya-biryukov marked an inline comment as done.
  • Forgotten change
clang-tools-extra/clangd/Protocol.cpp
707 ↗(On Diff #198307)

Thanks for catching that!
My mistake, I made the change but did not save the file.

ilya-biryukov planned changes to this revision.May 7 2019, 2:17 AM

Planning to rebase this on top of D61497 and land afterwards in order to minimize merge conflicts.

Ping, D61497 has landed

This is ready for review now

kadircet accepted this revision.May 29 2019, 2:40 AM

LGTM, thanks!

clang-tools-extra/clangd/XRefs.cpp
1170 ↗(On Diff #201844)

Should this also be inlinecode, for consistency? I know it is not exactly code as the other two cases, but it might still be better to look similar in all cases

This revision is now accepted and ready to land.May 29 2019, 2:40 AM
ilya-biryukov marked 2 inline comments as done.
  • Create an inline code block for 'global namespace' too
ilya-biryukov added inline comments.May 29 2019, 2:53 AM
clang-tools-extra/clangd/XRefs.cpp
1170 ↗(On Diff #201844)

Yeah, wasn't sure. Turned into inline code, could tweak later if needed.

This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptMay 29 2019, 3:00 AM