This is an archive of the discontinued LLVM Phabricator instance.

Differential D50230

clang-format: [JS] don't break comments before any '{'
ClosedPublic

Authored by mprobst on Aug 3 2018, 1:52 AM.

Details

Reviewers
krasimir
Commits
rGec45bc2feaa4: clang-format: [JS] don't break comments before any '{'
rL338837: clang-format: [JS] don't break comments before any '{'
rC338837: clang-format: [JS] don't break comments before any '{'
Summary

Previously, clang-format would avoid breaking before the first {
found, but then happily break before subsequent '{'s on the line. This
change fixes that by looking for the first location that has no opening
curly, if any.

Diff Detail

Repository
rC Clang

Event Timeline

mprobst created this revision.Aug 3 2018, 1:52 AM
Harbormaster completed remote builds in B21031: Diff 158945.Aug 3 2018, 1:52 AM
mprobst updated this revision to Diff 158946.Aug 3 2018, 1:57 AM

Also handle multiple numbered list tokens.

Harbormaster completed remote builds in B21032: Diff 158946.Aug 3 2018, 1:57 AM
krasimir accepted this revision.Aug 3 2018, 2:19 AM

Great! Thanks!

This revision is now accepted and ready to land.Aug 3 2018, 2:19 AM
Closed by commit rC338837: clang-format: [JS] don't break comments before any '{' (authored by mprobst). · Explain WhyAug 3 2018, 2:35 AM
This revision was automatically updated to reflect the committed changes.

Revision Contents

PathSize
lib/
Format/
24 lines
unittests/
Format/
8 lines

Diff 158957

lib/Format/BreakableToken.cpp

Show First 20 Lines • Show All 84 Lines • ▼ Show 20 Lines for (unsigned NumChars = 0;
NumChars += NumChars +=
encoding::columnWidthWithTabs(Text.substr(MaxSplitBytes, BytesInChar), encoding::columnWidthWithTabs(Text.substr(MaxSplitBytes, BytesInChar),
ContentStartColumn, TabWidth, Encoding); ContentStartColumn, TabWidth, Encoding);
MaxSplitBytes += BytesInChar; MaxSplitBytes += BytesInChar;
} }
StringRef::size_type SpaceOffset = Text.find_last_of(Blanks, MaxSplitBytes); StringRef::size_type SpaceOffset = Text.find_last_of(Blanks, MaxSplitBytes);
static auto *const kNumberedListRegexp = new llvm::Regex("^[1-9][0-9]?\\.");
while (SpaceOffset != StringRef::npos) {
// Do not split before a number followed by a dot: this would be interpreted // Do not split before a number followed by a dot: this would be interpreted
// as a numbered list, which would prevent re-flowing in subsequent passes. // as a numbered list, which would prevent re-flowing in subsequent passes.
static auto *const kNumberedListRegexp = new llvm::Regex("^[1-9][0-9]?\\."); if (kNumberedListRegexp->match(Text.substr(SpaceOffset).ltrim(Blanks)))
if (SpaceOffset != StringRef::npos &&
kNumberedListRegexp->match(Text.substr(SpaceOffset).ltrim(Blanks)))
SpaceOffset = Text.find_last_of(Blanks, SpaceOffset); SpaceOffset = Text.find_last_of(Blanks, SpaceOffset);
// In JavaScript, some @tags can be followed by {, and machinery that parses // In JavaScript, some @tags can be followed by {, and machinery that parses
// these comments will fail to understand the comment if followed by a line // these comments will fail to understand the comment if followed by a line
// break. So avoid ever breaking before a {. // break. So avoid ever breaking before a {.
if (Style.Language == FormatStyle::LK_JavaScript && else if (Style.Language == FormatStyle::LK_JavaScript &&
SpaceOffset != StringRef::npos && SpaceOffset + 1 < Text.size() && SpaceOffset + 1 < Text.size() && Text[SpaceOffset + 1] == '{')
Text[SpaceOffset + 1] == '{')
SpaceOffset = Text.find_last_of(Blanks, SpaceOffset); SpaceOffset = Text.find_last_of(Blanks, SpaceOffset);
}
if (SpaceOffset == StringRef::npos || if (SpaceOffset == StringRef::npos ||
// Don't break at leading whitespace. // Don't break at leading whitespace.
Text.find_last_not_of(Blanks, SpaceOffset) == StringRef::npos) { Text.find_last_not_of(Blanks, SpaceOffset) == StringRef::npos) {
// Make sure that we don't break at leading whitespace that // Make sure that we don't break at leading whitespace that
// reaches past MaxSplit. // reaches past MaxSplit.
StringRef::size_type FirstNonWhitespace = Text.find_first_not_of(Blanks); StringRef::size_type FirstNonWhitespace = Text.find_first_not_of(Blanks);
if (FirstNonWhitespace == StringRef::npos) if (FirstNonWhitespace == StringRef::npos)
▲ Show 20 Lines • Show All 853 Lines • Show Last 20 Lines

unittests/Format/FormatTestJS.cpp

Show First 20 Lines • Show All 2,061 Lines • ▼ Show 20 LinesTEST_F(FormatTestJS, JSDocAnnotations) {
verifyFormat("/**\n" verifyFormat("/**\n"
" * @param {canWrap\n" " * @param {canWrap\n"
" * onSpace}\n" " * onSpace}\n"
" */", " */",
"/**\n" "/**\n"
" * @param {canWrap onSpace}\n" " * @param {canWrap onSpace}\n"
" */", " */",
getGoogleJSStyleWithColumns(20)); getGoogleJSStyleWithColumns(20));
// make sure clang-format doesn't break before *any* '{'
verifyFormat("/**\n"
" * @lala {lala {lalala\n"
" */\n",
"/**\n"
" * @lala {lala {lalala\n"
" */\n",
getGoogleJSStyleWithColumns(20));
verifyFormat("/**\n" verifyFormat("/**\n"
" * @see http://very/very/long/url/is/long\n" " * @see http://very/very/long/url/is/long\n"
" */", " */",
"/**\n" "/**\n"
" * @see http://very/very/long/url/is/long\n" " * @see http://very/very/long/url/is/long\n"
" */", " */",
getGoogleJSStyleWithColumns(20)); getGoogleJSStyleWithColumns(20));
verifyFormat( verifyFormat(
▲ Show 20 Lines • Show All 203 Lines • Show Last 20 Lines