This is an archive of the discontinued LLVM Phabricator instance.

Differential D96896

[clang-format] Respect spaces in line comment section without an active column limit
ClosedPublic

Authored by HazardyKnusperkeks on Feb 17 2021, 1:19 PM.

Details

Reviewers
MyDeveloperDay
curdeius
Commits
rG418b4a7b3158: [clang-format] Respect spaces in line comment section...
Summary

Before line comments were not touched at all with ColumnLimit == 0, so this is a real change. But at least for me it was unexpected that the comments were not fixed with ColumnLimit == 0.

Diff Detail

Event Timeline

HazardyKnusperkeks requested review of this revision.Feb 17 2021, 1:19 PM
HazardyKnusperkeks created this revision.
Herald added a project: Restricted Project. · View Herald TranscriptFeb 17 2021, 1:19 PM
Herald added a subscriber: cfe-commits. · View Herald Transcript
Harbormaster completed remote builds in B89605: Diff 324410.Feb 17 2021, 3:20 PM
curdeius added a comment.Feb 18 2021, 9:10 AM

It seems to be a bug indeed.
So, basically all the changes are just adding a bunch of conditions on ColumnLimit != 0 which is a special value meaning "no limit", right? Or have I missed something?
Thining out loud... wouldn't it be easier to change ColumnLimit from 0 to numeric_limits<unsigned>::max() just before these transformations?

clang/lib/Format/ContinuationIndenter.cpp
2241

Is this condition on ColumnLimit (and other below) working ok when ColumnLimit == 0? Do your tests cover it?

HazardyKnusperkeks planned changes to this revision.Feb 18 2021, 11:17 AM
HazardyKnusperkeks added inline comments.
clang/lib/Format/ContinuationIndenter.cpp
2241

I must admit, that I don't know for sure.
Line comments should be checked, and a string literal on one line is also covered (that's how I found that I have to disable the inner loop). I should add a new test case with multiline comments and multiline string literals.

But I think your proposal of using ::max() is sound and should be used.

HazardyKnusperkeks updated this revision to Diff 326928.Feb 27 2021, 2:25 PM
  • Work with ::max instead of many checks for == 0.
  • Additional Testcase
Harbormaster completed remote builds in B91198: Diff 326928.Feb 27 2021, 2:58 PM
curdeius accepted this revision.Feb 28 2021, 1:43 AM

LGTM.

This revision is now accepted and ready to land.Feb 28 2021, 1:43 AM
This revision was landed with ongoing or failed builds.Mar 1 2021, 12:28 PM
Closed by commit rG418b4a7b3158: [clang-format] Respect spaces in line comment section... (authored by HazardyKnusperkeks). · Explain Why
This revision was automatically updated to reflect the committed changes.
HazardyKnusperkeks added a commit: rG418b4a7b3158: [clang-format] Respect spaces in line comment section....

Revision Contents

PathSize
clang/
lib/
Format/
5 lines
unittests/
Format/
27 lines
183 lines

Diff 327245

clang/lib/Format/ContinuationIndenter.cpp

Show First 20 Lines • Show All 1,987 Lines • ▼ Show 20 LinesContinuationIndenter::breakProtrudingToken(const FormatToken &Current,
if (!Token) if (!Token)
return {0, false}; return {0, false};
assert(Token->getLineCount() > 0); assert(Token->getLineCount() > 0);
unsigned ColumnLimit = getColumnLimit(State); unsigned ColumnLimit = getColumnLimit(State);
if (Current.is(TT_LineComment)) { if (Current.is(TT_LineComment)) {
// We don't insert backslashes when breaking line comments. // We don't insert backslashes when breaking line comments.
ColumnLimit = Style.ColumnLimit; ColumnLimit = Style.ColumnLimit;
} }
if (ColumnLimit == 0) {
// To make the rest of the function easier set the column limit to the
// maximum, if there should be no limit.
ColumnLimit = std::numeric_limits<decltype(ColumnLimit)>::max();
}
if (Current.UnbreakableTailLength >= ColumnLimit) if (Current.UnbreakableTailLength >= ColumnLimit)
return {0, false}; return {0, false};
// ColumnWidth was already accounted into State.Column before calling // ColumnWidth was already accounted into State.Column before calling
// breakProtrudingToken. // breakProtrudingToken.
unsigned StartColumn = State.Column - Current.ColumnWidth; unsigned StartColumn = State.Column - Current.ColumnWidth;
unsigned NewBreakPenalty = Current.isStringLiteral() unsigned NewBreakPenalty = Current.isStringLiteral()
? Style.PenaltyBreakString ? Style.PenaltyBreakString
: Style.PenaltyBreakComment; : Style.PenaltyBreakComment;
▲ Show 20 Lines • Show All 224 Lines • ▼ Show 20 Lines if (LineIndex + 1 != EndIndex) {
LLVM_DEBUG(llvm::dbgs() << SplitBeforeNext.first << ", " LLVM_DEBUG(llvm::dbgs() << SplitBeforeNext.first << ", "
<< SplitBeforeNext.second << "\n"); << SplitBeforeNext.second << "\n");
TailOffset = SplitBeforeNext.first + SplitBeforeNext.second; TailOffset = SplitBeforeNext.first + SplitBeforeNext.second;
// If the rest of the next line fits into the current line below the // If the rest of the next line fits into the current line below the
// column limit, we can safely reflow. // column limit, we can safely reflow.
RemainingTokenColumns = Token->getRemainingLength( RemainingTokenColumns = Token->getRemainingLength(
NextLineIndex, TailOffset, ContentStartColumn); NextLineIndex, TailOffset, ContentStartColumn);
Reflow = true; Reflow = true;
if (ContentStartColumn + RemainingTokenColumns > ColumnLimit) { if (ContentStartColumn + RemainingTokenColumns > ColumnLimit) {
curdeiusUnsubmitted
Not Done
ReplyInline Actions

Is this condition on ColumnLimit (and other below) working ok when ColumnLimit == 0? Do your tests cover it?

curdeius: Is this condition on `ColumnLimit` (and other below) working ok when `ColumnLimit == 0`? Do…
HazardyKnusperkeksAuthorUnsubmitted
Done
ReplyInline Actions

I must admit, that I don't know for sure.
Line comments should be checked, and a string literal on one line is also covered (that's how I found that I have to disable the inner loop). I should add a new test case with multiline comments and multiline string literals.

But I think your proposal of using ::max() is sound and should be used.

HazardyKnusperkeks: I must admit, that I don't know for sure. Line comments should be checked, and a string literal…
LLVM_DEBUG(llvm::dbgs() LLVM_DEBUG(llvm::dbgs()
<< " Over limit after reflow, need: " << " Over limit after reflow, need: "
<< (ContentStartColumn + RemainingTokenColumns) << (ContentStartColumn + RemainingTokenColumns)
<< ", space: " << ColumnLimit << ", space: " << ColumnLimit
<< ", reflown prefix: " << ContentStartColumn << ", reflown prefix: " << ContentStartColumn
<< ", offset in line: " << TailOffset << "\n"); << ", offset in line: " << TailOffset << "\n");
// If the whole next line does not fit, try to find a point in // If the whole next line does not fit, try to find a point in
// the next line at which we can break so that attaching the part // the next line at which we can break so that attaching the part
▲ Show 20 Lines • Show All 139 Lines • Show Last 20 Lines

clang/unittests/Format/FormatTest.cpp

  • This file is larger than 256 KB, so syntax highlighting is disabled by default.
Show First 20 Lines • Show All 19,247 Lines • ▼ Show 20 LinesTEST_F(FormatTest, IndentAccessModifiers) {
// also proves that the result is Style.AccessModifierOffset agnostic. // also proves that the result is Style.AccessModifierOffset agnostic.
Style.IndentWidth = 3; Style.IndentWidth = 3;
verifyFormat("class C {\n" verifyFormat("class C {\n"
" public:\n" " public:\n"
" int i;\n" " int i;\n"
"};\n", "};\n",
Style); Style);
} }
TEST_F(FormatTest, LimitlessStringsAndComments) {
auto Style = getLLVMStyleWithColumns(0);
constexpr StringRef Code =
"/**\n"
" * This is a multiline comment with quite some long lines, at least for "
"the LLVM Style.\n"
" * We will redo this with strings and line comments. Just to check if "
"everything is working.\n"
" */\n"
"bool foo() {\n"
" /* Single line multi line comment. */\n"
" const std::string String = \"This is a multiline string with quite "
"some long lines, at least for the LLVM Style.\"\n"
" \"We already did it with multi line "
"comments, and we will do it with line comments. Just to check if "
"everything is working.\";\n"
" // This is a line comment (block) with quite some long lines, at "
"least for the LLVM Style.\n"
" // We already did this with multi line comments and strings. Just to "
"check if everything is working.\n"
" const std::string SmallString = \"Hello World\";\n"
" // Small line comment\n"
" return String.size() > SmallString.size();\n"
"}";
EXPECT_EQ(Code, format(Code, Style));
}
} // namespace } // namespace
} // namespace format } // namespace format
} // namespace clang } // namespace clang

clang/unittests/Format/FormatTestComments.cpp

Show First 20 Lines • Show All 3,787 Lines • ▼ Show 20 Lines EXPECT_EQ("int i; // A Comment to be\n"
" // with indent\n" " // with indent\n"
"\n" "\n"
"int i;// A Comment to be moved\n" "int i;// A Comment to be moved\n"
" // with indent\n" " // with indent\n"
"\n" "\n"
"int i;// A Comment to be moved\n" "int i;// A Comment to be moved\n"
" // with indent\n", " // with indent\n",
Style)); Style));
Style = getLLVMStyleWithColumns(0);
EXPECT_EQ("// Free comment without space\n"
"\n"
"// Free comment with 3 spaces\n"
"\n"
"/// Free Doxygen without space\n"
"\n"
"/// Free Doxygen with 3 spaces\n"
"\n"
"/// A Doxygen Comment with a nested list:\n"
"/// - Foo\n"
"/// - Bar\n"
"/// - Baz\n"
"/// - End\n"
"/// of the inner list\n"
"/// .\n"
"/// .\n"
"\n"
"namespace Foo {\n"
"bool bar(bool b) {\n"
" bool ret1 = true; ///< Doxygenstyle without space\n"
" bool ret2 = true; ///< Doxygenstyle with 3 spaces\n"
" if (b) {\n"
" // Foo\n"
"\n"
" // In function comment\n"
" ret2 = false;\n"
" } // End of if\n"
"\n"
" // if (ret1) {\n"
" // return ret2;\n"
" // }\n"
"\n"
" // if (ret1) {\n"
" // return ret2;\n"
" // }\n"
"\n"
" return ret1 && ret2;\n"
"}\n"
"} // namespace Foo\n"
"\n"
"namespace Bar {\n"
"int foo();\n"
"} // namespace Bar\n"
"//@Nothing added because of the non ascii char\n"
"\n"
"//@ Nothing removed because of the non ascii char\n"
"\n"
"// Comment to move to the left\n"
"// But not this?\n"
"// @but this\n"
"\n"
"// Comment to move to the right\n"
"//@ this stays\n"
"\n"
"//} will not move\n"
"\n"
"// vv will only move\n"
"// } if the line above does\n",
format(Code, Style));
Style.SpacesInLineCommentPrefix = {0, 0};
EXPECT_EQ("//Free comment without space\n"
"\n"
"//Free comment with 3 spaces\n"
"\n"
"///Free Doxygen without space\n"
"\n"
"///Free Doxygen with 3 spaces\n"
"\n"
"///A Doxygen Comment with a nested list:\n"
"///- Foo\n"
"///- Bar\n"
"/// - Baz\n" // Here we keep the relative indentation
"/// - End\n"
"/// of the inner list\n"
"/// .\n"
"///.\n"
"\n"
"namespace Foo {\n"
"bool bar(bool b) {\n"
" bool ret1 = true; ///<Doxygenstyle without space\n"
" bool ret2 = true; ///<Doxygenstyle with 3 spaces\n"
" if (b) {\n"
" //Foo\n"
"\n"
" //In function comment\n"
" ret2 = false;\n"
" } //End of if\n"
"\n"
" //if (ret1) {\n"
" // return ret2;\n"
" //}\n"
"\n"
" //if (ret1) {\n"
" // return ret2;\n"
" //}\n"
"\n"
" return ret1 && ret2;\n"
"}\n"
"} //namespace Foo\n"
"\n"
"namespace Bar {\n"
"int foo();\n"
"} //namespace Bar\n"
"//@Nothing added because of the non ascii char\n"
"\n"
"//@ Nothing removed because of the non ascii char\n"
"\n"
"//Comment to move to the left\n"
"//But not this?\n"
"//@but this\n"
"\n"
"//Comment to move to the right\n"
"//@ this stays\n"
"\n"
"//} will not move\n"
"\n"
"//vv will only move\n"
"//} if the line above does\n",
format(Code, Style));
Style.SpacesInLineCommentPrefix = {2, -1u};
EXPECT_EQ("// Free comment without space\n"
"\n"
"// Free comment with 3 spaces\n"
"\n"
"/// Free Doxygen without space\n"
"\n"
"/// Free Doxygen with 3 spaces\n"
"\n"
"/// A Doxygen Comment with a nested list:\n"
"/// - Foo\n"
"/// - Bar\n"
"/// - Baz\n"
"/// - End\n"
"/// of the inner list\n"
"/// .\n"
"/// .\n"
"\n"
"namespace Foo {\n"
"bool bar(bool b) {\n"
" bool ret1 = true; ///< Doxygenstyle without space\n"
" bool ret2 = true; ///< Doxygenstyle with 3 spaces\n"
" if (b) {\n"
" // Foo\n"
"\n"
" // In function comment\n"
" ret2 = false;\n"
" } // End of if\n"
"\n"
" // if (ret1) {\n"
" // return ret2;\n"
" // }\n"
"\n"
" // if (ret1) {\n"
" // return ret2;\n"
" // }\n"
"\n"
" return ret1 && ret2;\n"
"}\n"
"} // namespace Foo\n"
"\n"
"namespace Bar {\n"
"int foo();\n"
"} // namespace Bar\n"
"//@Nothing added because of the non ascii char\n"
"\n"
"//@ Nothing removed because of the non ascii char\n"
"\n"
"// Comment to move to the left\n"
"// But not this?\n"
"// @but this\n"
"\n"
"// Comment to move to the right\n"
"//@ this stays\n"
"\n"
"//} will not move\n"
"\n"
"// vv will only move\n"
"// } if the line above does\n",
format(Code, Style));
} }
} // end namespace } // end namespace
} // end namespace format } // end namespace format
} // end namespace clang } // end namespace clang