[PATCH] D58547: [clangd] Introduce intermediate representation of formatted text

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov marked an inline comment as done.
ilya-biryukov added inline comments.


================
Comment at: clang-tools-extra/clangd/ClangdServer.h:186
   void findHover(PathRef File, Position Pos,
-                 Callback<llvm::Optional<Hover>> CB);
+                 Callback<llvm::Optional<FormattedString>> CB);
 
----------------
kadircet wrote:
> sammccall wrote:
> > Not sure about switching from `Hover` to `FormattedString` as return type here: LSP hover struct contains a range field (what are the bounds of the hovered thing?) that we may want to populate that doesn't fit in FormattedString.
> > I'd suggest the function in XRefs.h should return `FormattedString` (and later maybe `pair<FormattedString, Range>`), and the `ClangdServer` version should continue to provide the rendered `Hover`.
> > 
> > (We'll eventually want ClangdServer to return some HoverInfo struct with structured information, as we want embedding clients to be able to render it other ways, and maybe use it to provide extension methods like YCM's `GetType`. But no need to touch that in this patch, we'll end up producing HoverInfo -> FormattedString -> LSP Hover, I guess)
> I agree with Sam on the layering. I was also working in a patch that was changing `ClangdServer`s output from `Hover` to `HoverInfo`(a structured version of `Hover`).
`ClangdServer` does not know whether to render the `FormattedString` into markdown or plaintext and I believe it should stay that way.
Happy to return `HoverInfo`, though. I'll introduce one with `FormattedString` and `Range`


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D58547