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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
