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

[Kadir Cetinkaya via Phabricator via llvm-commits]

kadircet added a comment.

mostly nits, only important comment is around ordering of include in the rendered hover response



================
Comment at: clang-tools-extra/clangd/FormattedString.cpp:148
+    bool IsBlock = C.Kind == ChunkKind::CodeBlock;
+    if (LastWasBlock.hasValue() && (IsBlock || *LastWasBlock))
+      if (!llvm::StringRef(C.Contents).endswith("\n\n"))
----------------
could you add braces to outermost `if`, and clang-format this chunk.


================
Comment at: clang-tools-extra/clangd/FormattedString.cpp:148
+    bool IsBlock = C.Kind == ChunkKind::CodeBlock;
+    if (LastWasBlock.hasValue() && (IsBlock || *LastWasBlock))
+      if (!llvm::StringRef(C.Contents).endswith("\n\n"))
----------------
kadircet wrote:
> could you add braces to outermost `if`, and clang-format this chunk.
nit: if the following change(regarding cleaning whitespaces at the end of each chunk) was unintentional,
why not make this one a lambda(like ensurewhitespaces) and just call it before and after we insert a `codeblock`s contents?


================
Comment at: clang-tools-extra/clangd/FormattedString.cpp:170
     }
-    llvm_unreachable("unhanlded ChunkKind");
+    while (!R.empty() && isWhitespace(R.back()))
+      R.pop_back();
----------------
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?


================
Comment at: clang-tools-extra/clangd/XRefs.cpp:616
+  if (!SymbolCollector::shouldCollectSymbol(ND, ND.getASTContext(),
+                                            SymbolCollector::Options(), false))
+    return;
----------------
/*IsMainFileOnly=*/false


================
Comment at: clang-tools-extra/clangd/XRefs.cpp:616
+  if (!SymbolCollector::shouldCollectSymbol(ND, ND.getASTContext(),
+                                            SymbolCollector::Options(), false))
+    return;
----------------
kadircet wrote:
> /*IsMainFileOnly=*/false
also could you add a comment explaining why it is checked for `IsMainFileOnly=false`. 

IIRC we decided that it was because, main file symbols' comments should already exist in the ast.


================
Comment at: clang-tools-extra/clangd/XRefs.cpp:736
   if (D)
+  {
     HI.Kind = indexSymbolKindToSymbolKind(index::getSymbolInfo(D).Kind);
----------------
clang-format


================
Comment at: clang-tools-extra/clangd/XRefs.cpp:1257
+
+  if (!Documentation.empty())
+  {
----------------
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?


================
Comment at: clang-tools-extra/clangd/XRefs.cpp:1258
+  if (!Documentation.empty())
+  {
+    Output.appendText(Documentation);
----------------
clang-format


Repository:
  rL LLVM

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

https://reviews.llvm.org/D64296