[PATCH] D64296: [clangd] Show documentation in hover, and fetch docs from index if needed.

[Kadir Cetinkaya via Phabricator via llvm-commits]

kadircet accepted this revision.
kadircet added a comment.
This revision is now accepted and ready to land.

LGTM, thanks!



================
Comment at: clang-tools-extra/clangd/FormattedString.cpp:170
     }
-    llvm_unreachable("unhanlded ChunkKind");
+    while (!R.empty() && isWhitespace(R.back()))
+      R.pop_back();
----------------
sammccall wrote:
> kadircet wrote:
> > why perform this for every chunk? This was previously used to get rid of trailing whitespaces, now this also trims trailing whitespaces in chunks. this doesn't sound valid to me?
> This normalizes the whitespace between inline chunks to one space. Note that the text/inline blocks ensure there's a space before following another inline chunk. Is there a case where we'd be adding inline text and *want* multiple spaces between chunks?
> 
> This was an incomplete thought though: it allows the \n\n block to be simplified.
> 
> (I'm not totally happy with this rendering logic either, but I'm fairly convinced we're going to have to replace FormattedString with something hierarchical before too long, so I'm trying to avoid doing too much surgery here)
> This normalizes the whitespace between inline chunks to one space. Note that the text/inline blocks ensure there's a space before following another inline chunk. Is there a case where we'd be adding inline text and *want* multiple spaces between chunks?

I am not sure if we would ever want to put some spaces between chunks, but it is still doable by prepending it to the second chunk instead of appending to the first.

So, I suppose it is OK until we figure out some other use case.


================
Comment at: clang-tools-extra/clangd/XRefs.cpp:1257
+
+  if (!Documentation.empty())
+  {
----------------
sammccall wrote:
> kadircet wrote:
> > I believe documentation should come first. Since it has a non-fixed length, I believe it would be nice if people could simply not notice it(assuming bottom is closer to cursor location) WDYT?
> The problem is that the hover text is often limited length and will almost certainly be truncated at the bottom. Hover also sometimes appears below the cursor.
> 
> Prioritizing from bottom->top is an interesting idea, but I don't think editors or users are expecting it.
you are right, truncation is definitely a bigger concern


Repository:
  rL LLVM

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

https://reviews.llvm.org/D64296