lolleko updated this revision to Diff 252006.
lolleko added a comment.
Adressed 2nd Review
The problem with the changes you proposed is that it doesn't handle paragraphs
(\n\n).
I think the conditionals required to make that work wouldn't be much cleaner
than the current solution.
And I do think parahraphs in comments should be retained. I agree that not
every hard line break should be a pargraph.
But actual parahraphs should be retained e.g.:
/**
* Condition for calling UpdateOverlaps() to initialize overlap state
when loaded in during level streaming.
* If set to 'UseConfigDefault', the default specified in ini
(displayed in 'DefaultUpdateOverlapsMethodDuringLevelStreaming') will be used.
* If overlaps are not initialized, this actor and attached components
will not have an initial state of what objects are touching it,
* and overlap events may only come in once one of those objects update
overlaps themselves (for example when moving).
* However if an object touching it *does* initialize state, both
objects will know about their touching state with each other.
* This can be a potentially large performance savings during level
streaming, and is safe if the object and others initially
* overlapping it do not need the overlap state because they will not
trigger overlap notifications.
*
* Note that if 'bGenerateOverlapEventsDuringLevelStreaming' is true,
overlaps are always updated in this case, but that flag
* determines whether the Begin/End overlap events are triggered.
*
* @see bGenerateOverlapEventsDuringLevelStreaming,
DefaultUpdateOverlapsMethodDuringLevelStreaming,
GetUpdateOverlapsMethodDuringLevelStreaming()
*/
I think once proper paragraphs are in. The linebreak parsing could be rewritten
into 2 steps:
1. Split on `\n\n` to extract actual paragraphs.
2. Use the solution you proposed to split and join new lines.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D76094/new/
https://reviews.llvm.org/D76094
Files:
clang-tools-extra/clangd/Hover.cpp
clang-tools-extra/clangd/Hover.h
clang-tools-extra/clangd/unittests/HoverTests.cpp
Index: clang-tools-extra/clangd/unittests/HoverTests.cpp
===================================================================
--- clang-tools-extra/clangd/unittests/HoverTests.cpp
+++ clang-tools-extra/clangd/unittests/HoverTests.cpp
@@ -1883,6 +1883,119 @@
}
}
+TEST(Hover, DocCommentLineBreakConversion) {
+ struct Case {
+ llvm::StringRef Documentation;
+ llvm::StringRef ExpectedRenderMarkdown;
+ llvm::StringRef ExpectedRenderPlainText;
+ } Cases[] = {{
+ "foo\n\n\nbar",
+ "foo \nbar",
+ "foo\nbar",
+ },
+ {
+ "foo\n\n\n\tbar",
+ "foo \nbar",
+ "foo\nbar",
+ },
+ {
+ "foo\n\n\n bar",
+ "foo \nbar",
+ "foo\nbar",
+ },
+ {
+ "foo.\nbar",
+ "foo. \nbar",
+ "foo.\nbar",
+ },
+ {
+ "foo:\nbar",
+ "foo: \nbar",
+ "foo:\nbar",
+ },
+ {
+ "foo,\nbar",
+ "foo, \nbar",
+ "foo,\nbar",
+ },
+ {
+ "foo;\nbar",
+ "foo; \nbar",
+ "foo;\nbar",
+ },
+ {
+ "foo!\nbar",
+ "foo! \nbar",
+ "foo!\nbar",
+ },
+ {
+ "foo?\nbar",
+ "foo? \nbar",
+ "foo?\nbar",
+ },
+ {
+ "foo\n-bar",
+ "foo \n-bar",
+ "foo\n-bar",
+ },
+ {
+ "foo\n*bar",
+ // TODO `*` should probably not be escaped after line break
+ "foo \n\\*bar",
+ "foo\n*bar",
+ },
+ {
+ "foo\n@bar",
+ "foo \n@bar",
+ "foo\n@bar",
+ },
+ {
+ "foo\n\\bar",
+ // TODO `\` should probably not be escaped after line break
+ "foo \n\\\\bar",
+ "foo\n\\bar",
+ },
+ {
+ "foo\n>bar",
+ // TODO `>` should probably not be escaped after line break
+ "foo \n\\>bar",
+ "foo\n>bar",
+ },
+ {
+ "foo\n#bar",
+ "foo \n#bar",
+ "foo\n#bar",
+ },
+ {
+ "foo \nbar",
+ "foo \nbar",
+ "foo\nbar",
+ },
+ {
+ "foo \nbar",
+ "foo \nbar",
+ "foo\nbar",
+ },
+ {
+ "foo\\\nbar",
+ "foo \nbar",
+ "foo\nbar",
+ },
+ {
+ "foo\nbar",
+ "foo bar",
+ "foo bar",
+ }};
+
+ for (const auto &C : Cases) {
+ markup::Document Output;
+ parseLineBreaks(C.Documentation, Output);
+
+ EXPECT_EQ(Output.asMarkdown(), C.ExpectedRenderMarkdown);
+ EXPECT_EQ(Output.asPlainText(), C.ExpectedRenderPlainText);
+ }
+}
+
// This is a separate test as headings don't create any differences in plaintext
// mode.
TEST(Hover, PresentHeadings) {
Index: clang-tools-extra/clangd/Hover.h
===================================================================
--- clang-tools-extra/clangd/Hover.h
+++ clang-tools-extra/clangd/Hover.h
@@ -74,6 +74,9 @@
/// Produce a user-readable information.
markup::Document present() const;
};
+
+void parseLineBreaks(llvm::StringRef Input, markup::Document &Output);
+
llvm::raw_ostream &operator<<(llvm::raw_ostream &, const HoverInfo::Param &);
inline bool operator==(const HoverInfo::Param &LHS,
const HoverInfo::Param &RHS) {
Index: clang-tools-extra/clangd/Hover.cpp
===================================================================
--- clang-tools-extra/clangd/Hover.cpp
+++ clang-tools-extra/clangd/Hover.cpp
@@ -520,6 +520,61 @@
}
return llvm::None;
}
+
+bool isParagraphLineBreak(llvm::StringRef Str, size_t LineBreakIndex) {
+ if (LineBreakIndex + 1 >= Str.size()) {
+ return false;
+ }
+ auto NextNonSpaceCharIndex = Str.find_first_not_of(' ', LineBreakIndex + 1);
+
+ return NextNonSpaceCharIndex != llvm::StringRef::npos &&
+ Str[NextNonSpaceCharIndex] == '\n';
+};
+
+bool isMardkownLineBreak(llvm::StringRef Str, size_t LineBreakIndex) {
+ return LineBreakIndex > 1 &&
+ (Str[LineBreakIndex - 1] == '\\' ||
+ (Str[LineBreakIndex - 1] == ' ' && Str[LineBreakIndex - 2] == ' '));
+};
+
+bool isPunctuationLineBreak(llvm::StringRef Str, size_t LineBreakIndex) {
+ constexpr llvm::StringLiteral Punctuation = R"txt(.:,;!?)txt";
+
+ return LineBreakIndex > 0 &&
+ Punctuation.find(Str[LineBreakIndex - 1]) != llvm::StringRef::npos;
+};
+
+bool isFollowedByHardLineBreakIndicator(llvm::StringRef Str,
+ size_t LineBreakIndex) {
+ // '-'/'*' md list, '@'/'\' documentation command, '>' md blockquote,
+ // '#' headings, '`' code blocks
+ constexpr llvm::StringLiteral LinbreakIdenticators = R"txt(-*@\>#`)txt";
+
+ auto NextNonSpaceCharIndex = Str.find_first_not_of(' ', LineBreakIndex + 1);
+
+ if (NextNonSpaceCharIndex == llvm::StringRef::npos) {
+ return false;
+ }
+
+ auto FollowedBySingleCharIndicator =
+ LinbreakIdenticators.find(Str[NextNonSpaceCharIndex]) !=
+ llvm::StringRef::npos;
+
+ auto FollowedByNumberedListIndicator =
+ llvm::isDigit(Str[NextNonSpaceCharIndex]) &&
+ NextNonSpaceCharIndex + 1 < Str.size() &&
+ (Str[NextNonSpaceCharIndex + 1] == '.' ||
+ Str[NextNonSpaceCharIndex + 1] == ')');
+
+ return FollowedBySingleCharIndicator || FollowedByNumberedListIndicator;
+};
+
+bool isHardLineBreak(llvm::StringRef Str, size_t LineBreakIndex) {
+ return isMardkownLineBreak(Str, LineBreakIndex) ||
+ isPunctuationLineBreak(Str, LineBreakIndex) ||
+ isFollowedByHardLineBreakIndicator(Str, LineBreakIndex);
+}
+
} // namespace
llvm::Optional<HoverInfo> getHover(ParsedAST &AST, Position Pos,
@@ -652,7 +707,7 @@
}
if (!Documentation.empty())
- Output.addParagraph().appendText(Documentation);
+ parseLineBreaks(Documentation, Output);
if (!Definition.empty()) {
Output.addRuler();
@@ -675,6 +730,49 @@
return Output;
}
+// Tries to retain hard line breaks and drops soft line breaks.
+void parseLineBreaks(llvm::StringRef Input, markup::Document &Output) {
+
+ constexpr auto WhiteSpaceChars = "\t\n\v\f\r ";
+
+ auto TrimmedInput = Input.trim();
+
+ std::string CurrentLine;
+
+ for (size_t CharIndex = 0; CharIndex < TrimmedInput.size();) {
+ if (TrimmedInput[CharIndex] == '\n') {
+ // Trim whitespace infront of linebreak, also get rif of markdown
+ // linebreak character `\`
+ const auto LastNonSpaceCharIndex =
+ CurrentLine.find_last_not_of(std::string(WhiteSpaceChars) + "\\") + 1;
+ CurrentLine.erase(LastNonSpaceCharIndex);
+
+ if (isParagraphLineBreak(TrimmedInput, CharIndex)) {
+ // TODO this should add an actual paragraph (double linebreak)
+ Output.addParagraph().appendText(CurrentLine);
+ CurrentLine = "";
+ } else if (isHardLineBreak(TrimmedInput, CharIndex)) {
+ Output.addParagraph().appendText(CurrentLine);
+ CurrentLine = "";
+ } else {
+ // Ommit linebreak
+ CurrentLine += ' ';
+ }
+
+ CharIndex++;
+ // After a linebreak always remove spaces to avoid 4 space markdown code
+ // blocks, also skip all additional linebreaks since they have no effect
+ CharIndex = TrimmedInput.find_first_not_of(WhiteSpaceChars, CharIndex);
+ } else {
+ CurrentLine += TrimmedInput[CharIndex];
+ CharIndex++;
+ }
+ }
+ if (!CurrentLine.empty()) {
+ Output.addParagraph().appendText(CurrentLine);
+ }
+}
+
llvm::raw_ostream &operator<<(llvm::raw_ostream &OS,
const HoverInfo::Param &P) {
std::vector<llvm::StringRef> Output;
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
