[PATCH] D76094: [clangd] Change line break behaviour for hoverinfo

[Sam McCall via Phabricator via cfe-commits]

sammccall added a comment.

Thanks, this fits in a lot better!
I think there's some more details that could be handled, e.g. if a line is much shorter than the max line length, then that probably implies it's a "real" break. But if we get the code structure right, these should be easy to plug in later.

> Because the current implementation of markup::Paragraph only appends a single white space after it's contents. I think this is semantically wrong because in natural language aswell as markup languages like md, html, ... a paragraph should be followed by a blank line

Yeah, we can look at this again, but we shouldn't do so in this patch. You're right on the semantics, but real implementations (particularly VSCode) use a distasteful amount of whitespace around HTML paragraphs vs other UI elements and don't give us any control over the CSS. (Similarly rendering paragraph breaks as `\n\n` in plaintext feels much too heavyweight in terminal-based editors in my experience)

BTW, it's really useful to upload diffs with full file context, I think -U99999 will do this, though I tend to use `arc diff` which always includes it.



================
Comment at: clang-tools-extra/clangd/Hover.cpp:524
+
+// Parses in documentation comments and tries to preserve the markup in the
+// comment.
----------------
I think we're going to end up using this elsewhere, e.g. in code completion.
Fine to keep it in Hover for now, but please expose it from hover.h and test it directly (rather than via HoverInfo::present())

This will also allow moving this implementation to the end of the file, rather than interleaving it with unrelated parts of hover logic.


================
Comment at: clang-tools-extra/clangd/Hover.cpp:527
+// Currently only linebreaks are handled.
+void parseDocumentation(std::string Input, markup::Document &Output) {
+
----------------
nit: input should be StringRef since you never mutate it


================
Comment at: clang-tools-extra/clangd/Hover.cpp:529
+
+  auto IsParagraphLineBreak = [](llvm::StringRef Str, size_t LineBreakIndex) {
+    if (LineBreakIndex + 1 >= Str.size()) {
----------------
nit: we'd generally use static/anon-namespace functions rather than lambdas for helpers when we don't need to capture much stuff.


================
Comment at: clang-tools-extra/clangd/Hover.cpp:583
+
+  for (size_t CharIndex = 0; CharIndex < TrimmedInput.size();) {
+    if (TrimmedInput[CharIndex] == '\n') {
----------------
I think this could be easier to read by separating the strategy from some of the details.

>From my reading of the code, the strategy is:
 - examine the raw lines from the input to determine where true paragraph breaks occur. (In general, based on the end of the previous line and the start of the new line)
 - form each groups of raw lines into a paragraph (by trimming whitespace and `\`, dropping blank lines, and joining the results with spaces)

Given this, I think we could extract a couple of functions like:
 - `bool breakBetween(StringRef Before, StringRef After)`
 - `std::string joinRawLines(ArrayRef<StringRef>)`

and the outer loop becomes simpler, like:
```
SmallVector<StringRef, N> RawLines;
llvm::SplitString(Input, RawLines, "\n");
ArrayRef Remaining = RawLines;
while (!Remaining.empty()) {
  int Para = 1;
  while (Para < Remaining.size() && !breakBetween(Remaining[Para-1], Remaining[Para]))
    Para++;
  std::string ParaText = joinRawLines(Remaining.take_front(Para));
  if (!ParaText.empty())
    Output.addParagraph().appendText(std::move(ParaText));
  Remaining = Remaining.drop_front(Para);
}
```

I think this signature for `breakBetween` in terms of stringrefs would result in the lambdas you have here being a bit more readable, and the main is clearer for not dealing directly with string, whitespace, etc.


================
Comment at: clang-tools-extra/clangd/unittests/HoverTests.cpp:1886
 
+TEST(Hover, LineBreakConversionDocs) {
+  struct Case {
----------------
@kadircet this feels like a case where having a debug output mode like `[Para:foo][Para:bar]` would be useful!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76094