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

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet 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);
 
----------------
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`).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D58547