Skip to content

Commit 9d71781

Browse files
mprobstmprobst
committedAug 2, 2018
clang-format: fix a crash in comment wraps.
Summary: Previously, clang-format would crash if it tried to wrap an overlong single line comment, because two parts of the code inserted a break in the same location. /** heregoesalongcommentwithnospace */ This wasn't previously noticed as it could only trigger for an overlong single line comment that did have no breaking opportunities except for a whitespace at the very beginning. This also introduces a check for JavaScript to not ever wrap a comment before an opening curly brace: /** @mods {donotbreakbeforethecurly} */ This is because some machinery parsing these tags sometimes supports breaks before a possible `{`, but in some other cases does not. Previously clang-format was careful never to wrap a line with certain tags on it. The better solution is to specifically disable wrapping before the problematic token: this allows wrapping and aligning comments but still avoids the problem. Reviewers: krasimir Subscribers: cfe-commits Differential Revision: https://reviews.llvm.org/D50177 llvm-svn: 338706
·
llvmorg-21-initllvmorg-8.0.0-rc1
1 parent 1a721eb commit 9d71781

File tree

3 files changed

+52
-25
lines changed

3 files changed

+52
-25
lines changed
 

‎clang/lib/Format/BreakableToken.cpp

+21-5
Original file line numberDiff line numberDiff line change
@@ -67,10 +67,11 @@ static BreakableToken::Split getCommentSplit(StringRef Text,
6767
unsigned ContentStartColumn,
6868
unsigned ColumnLimit,
6969
unsigned TabWidth,
70-
encoding::Encoding Encoding) {
71-
LLVM_DEBUG(llvm::dbgs() << "Comment split: \"" << Text << ", " << ColumnLimit
72-
<< "\", Content start: " << ContentStartColumn
73-
<< "\n");
70+
encoding::Encoding Encoding,
71+
const FormatStyle &Style) {
72+
LLVM_DEBUG(llvm::dbgs() << "Comment split: \"" << Text
73+
<< "\", Column limit: " << ColumnLimit
74+
<< ", Content start: " << ContentStartColumn << "\n");
7475
if (ColumnLimit <= ContentStartColumn + 1)
7576
return BreakableToken::Split(StringRef::npos, 0);
7677

@@ -95,6 +96,13 @@ static BreakableToken::Split getCommentSplit(StringRef Text,
9596
if (SpaceOffset != StringRef::npos &&
9697
kNumberedListRegexp->match(Text.substr(SpaceOffset).ltrim(Blanks)))
9798
SpaceOffset = Text.find_last_of(Blanks, SpaceOffset);
99+
// In JavaScript, some @tags can be followed by {, and machinery that parses
100+
// these comments will fail to understand the comment if followed by a line
101+
// break. So avoid ever breaking before a {.
102+
if (Style.Language == FormatStyle::LK_JavaScript &&
103+
SpaceOffset != StringRef::npos && SpaceOffset + 1 < Text.size() &&
104+
Text[SpaceOffset + 1] == '{')
105+
SpaceOffset = Text.find_last_of(Blanks, SpaceOffset);
98106

99107
if (SpaceOffset == StringRef::npos ||
100108
// Don't break at leading whitespace.
@@ -109,6 +117,12 @@ static BreakableToken::Split getCommentSplit(StringRef Text,
109117
Blanks, std::max<unsigned>(MaxSplitBytes, FirstNonWhitespace));
110118
}
111119
if (SpaceOffset != StringRef::npos && SpaceOffset != 0) {
120+
// adaptStartOfLine will break after lines starting with /** if the comment
121+
// is broken anywhere. Avoid emitting this break twice here.
122+
// Example: in /** longtextcomesherethatbreaks */ (with ColumnLimit 20) will
123+
// insert a break after /**, so this code must not insert the same break.
124+
if (SpaceOffset == 1 && Text[SpaceOffset - 1] == '*')
125+
return BreakableToken::Split(StringRef::npos, 0);
112126
StringRef BeforeCut = Text.substr(0, SpaceOffset).rtrim(Blanks);
113127
StringRef AfterCut = Text.substr(SpaceOffset).ltrim(Blanks);
114128
return BreakableToken::Split(BeforeCut.size(),
@@ -260,7 +274,7 @@ BreakableComment::getSplit(unsigned LineIndex, unsigned TailOffset,
260274
return Split(StringRef::npos, 0);
261275
return getCommentSplit(Content[LineIndex].substr(TailOffset),
262276
ContentStartColumn, ColumnLimit, Style.TabWidth,
263-
Encoding);
277+
Encoding, Style);
264278
}
265279

266280
void BreakableComment::compressWhitespace(
@@ -620,6 +634,8 @@ void BreakableBlockComment::adaptStartOfLine(
620634
if (DelimitersOnNewline) {
621635
// Since we're breaking at index 1 below, the break position and the
622636
// break length are the same.
637+
// Note: this works because getCommentSplit is careful never to split at
638+
// the beginning of a line.
623639
size_t BreakLength = Lines[0].substr(1).find_first_not_of(Blanks);
624640
if (BreakLength != StringRef::npos)
625641
insertBreak(LineIndex, 0, Split(1, BreakLength), /*ContentIndent=*/0,

‎clang/unittests/Format/FormatTestComments.cpp

+6
Original file line numberDiff line numberDiff line change
@@ -1254,6 +1254,12 @@ TEST_F(FormatTestComments, SplitsLongLinesInComments) {
12541254
" */",
12551255
getLLVMStyleWithColumns(20)));
12561256

1257+
// This reproduces a crashing bug where both adaptStartOfLine and
1258+
// getCommentSplit were trying to wrap after the "/**".
1259+
EXPECT_EQ("/** multilineblockcommentwithnowrapopportunity */",
1260+
format("/** multilineblockcommentwithnowrapopportunity */",
1261+
getLLVMStyleWithColumns(20)));
1262+
12571263
EXPECT_EQ("/*\n"
12581264
"\n"
12591265
"\n"

‎clang/unittests/Format/FormatTestJS.cpp

+25-20
Original file line numberDiff line numberDiff line change
@@ -191,31 +191,32 @@ TEST_F(FormatTestJS, JSDocComments) {
191191

192192
// Break a single line long jsdoc comment pragma.
193193
EXPECT_EQ("/**\n"
194-
" * @returns\n"
195-
" * {string}\n"
196-
" * jsdoc line 12\n"
194+
" * @returns {string} jsdoc line 12\n"
197195
" */",
198196
format("/** @returns {string} jsdoc line 12 */",
199197
getGoogleJSStyleWithColumns(20)));
200-
201198
EXPECT_EQ("/**\n"
202-
" * @returns\n"
203-
" * {string}\n"
199+
" * @returns {string}\n"
204200
" * jsdoc line 12\n"
205201
" */",
202+
format("/** @returns {string} jsdoc line 12 */",
203+
getGoogleJSStyleWithColumns(25)));
204+
205+
EXPECT_EQ("/**\n"
206+
" * @returns {string} jsdoc line 12\n"
207+
" */",
206208
format("/** @returns {string} jsdoc line 12 */",
207209
getGoogleJSStyleWithColumns(20)));
208210

209211
// FIXME: this overcounts the */ as a continuation of the 12 when breaking.
210212
// Related to the FIXME in BreakableBlockComment::getRangeLength.
211213
EXPECT_EQ("/**\n"
212-
" * @returns\n"
213-
" * {string}\n"
214-
" * jsdoc line\n"
214+
" * @returns {string}\n"
215+
" * jsdoc line line\n"
215216
" * 12\n"
216217
" */",
217-
format("/** @returns {string} jsdoc line 12*/",
218-
getGoogleJSStyleWithColumns(20)));
218+
format("/** @returns {string} jsdoc line line 12*/",
219+
getGoogleJSStyleWithColumns(25)));
219220

220221
// Fix a multiline jsdoc comment ending in a comment pragma.
221222
EXPECT_EQ("/**\n"
@@ -2038,27 +2039,32 @@ TEST_F(FormatTestJS, WrapAfterParen) {
20382039

20392040
TEST_F(FormatTestJS, JSDocAnnotations) {
20402041
verifyFormat("/**\n"
2041-
" * @exports\n"
2042-
" * {this.is.a.long.path.to.a.Type}\n"
2042+
" * @exports {this.is.a.long.path.to.a.Type}\n"
20432043
" */",
20442044
"/**\n"
20452045
" * @exports {this.is.a.long.path.to.a.Type}\n"
20462046
" */",
20472047
getGoogleJSStyleWithColumns(20));
20482048
verifyFormat("/**\n"
2049-
" * @mods\n"
2050-
" * {this.is.a.long.path.to.a.Type}\n"
2049+
" * @mods {this.is.a.long.path.to.a.Type}\n"
2050+
" */",
2051+
"/**\n"
2052+
" * @mods {this.is.a.long.path.to.a.Type}\n"
2053+
" */",
2054+
getGoogleJSStyleWithColumns(20));
2055+
verifyFormat("/**\n"
2056+
" * @mods {this.is.a.long.path.to.a.Type}\n"
20512057
" */",
20522058
"/**\n"
20532059
" * @mods {this.is.a.long.path.to.a.Type}\n"
20542060
" */",
20552061
getGoogleJSStyleWithColumns(20));
20562062
verifyFormat("/**\n"
2057-
" * @param\n"
2058-
" * {this.is.a.long.path.to.a.Type}\n"
2063+
" * @param {canWrap\n"
2064+
" * onSpace}\n"
20592065
" */",
20602066
"/**\n"
2061-
" * @param {this.is.a.long.path.to.a.Type}\n"
2067+
" * @param {canWrap onSpace}\n"
20622068
" */",
20632069
getGoogleJSStyleWithColumns(20));
20642070
verifyFormat("/**\n"
@@ -2083,8 +2089,7 @@ TEST_F(FormatTestJS, JSDocAnnotations) {
20832089
" /**\n"
20842090
" * long long long\n"
20852091
" * long\n"
2086-
" * @param\n"
2087-
" * {this.is.a.long.path.to.a.Type}\n"
2092+
" * @param {this.is.a.long.path.to.a.Type}\n"
20882093
" * a\n"
20892094
" * long long long\n"
20902095
" * long long\n"

0 commit comments

Comments
 (0)
Please sign in to comment.