[PATCH] D61601: [clangd] Represent Hover result using FormattedString

Tue May 7 00:48:37 PDT 2019

kadircet added a comment.

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 <https://reviews.llvm.org/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.

================
Comment at: clang-tools-extra/clangd/ClangdLSPServer.cpp:853
+                          Hover R;
+                          switch (HoverContentFormat) {
+                          case MarkupKind::Plaintext:
----------------
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.

================
Comment at: clang-tools-extra/clangd/Protocol.cpp:707
+  else
+    return false;
+  return true;
----------------
Maybe also vlog/elog the unknown kind? So that we can easily catch new additions to spec.

================
Comment at: clang-tools-extra/clangd/Protocol.h:359
+enum class MarkupKind {
+  Plaintext,
+  Markdown,
----------------
This is mentioned as `PlainText` in the specs

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D61601/new/

https://reviews.llvm.org/D61601