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

[Sam McCall via Phabricator via cfe-commits]

sammccall added a comment.

I understand the main goal here is preserving intended line breaks in documentation comments? Thanks for tackling this problem!

A design goal here is that the markup objects are the source of truth about the document structure. So if we've decided we want a comment to turn into multiple lines, we should create multiple Paragraph objects, this makes it easy to adjust the rendering strategy and isolate differences between text/markdown.

I think this means this splitting needs to happen on the way in instead of on the way out. In Hover.cpp:

  if (!Documentation.empty())
    Output.addParagraph().appendText(Documentation);

would be come something like

  parseDocumentationComment(Output, Documentation);

which would ultimately add a paragraph to `Output` for each line detected in Documentation.

> Even though markdown doc comments in cpp are still rare, I removed the markdown escaping

Please let's leave this out, at least from this patch, if it's not the primary goal. This is a complicated tradeoff, there's plenty of reasonably common patterns where we should be escaping, such as `vector<string>`. Moreover if we're treating punctuation in the comments as formatting, we should likely also be doing so in plain-text mode.

FYI D75687 <https://reviews.llvm.org/D75687> (less unneccesary escaping in markdown) is also in flight and likely to touch some of the same code.



================
Comment at: clang-tools-extra/clangd/FormattedString.cpp:103
+// converted to markdown linebreaks if \p Markdown is set.
+std::string convertLineBreaks(llvm::StringRef Input, bool Markdown = false) {
+  Input = Input.trim();
----------------
We can separate concerns more clearly by not having this function care about markdown.
Can it have a signature like:
 - `vector<StringRef> splitLines(StringRef Text)` (which wouldn't normalize whitespace within the lines)
 - `vector<string> splitLines(StringRef Text)` which would
and then continue to have line rendering handled elsewhere?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76094