This is an archive of the discontinued LLVM Phabricator instance.

Differential D35557

clang-format: merge short case labels with trailing comments
ClosedPublic

Authored by Typz on Jul 18 2017, 5:53 AM.

Details

Reviewers
krasimir
djasper
Commits
rGa64ba70183c3: clang-format: merge short case labels with trailing comments
rC309370: clang-format: merge short case labels with trailing comments
rL309370: clang-format: merge short case labels with trailing comments
Summary

Allow merging short case labels when they actually end with a comment
(like a comment after the `break`) and when followed by switch-level
comments (e.g. aligned with next case):

switch(a) {
case 0: break; // comment at end of case
case 1: return value;
// comment related to next case
// comment related to next case
case 2:
}

Diff Detail

Event Timeline

Typz created this revision.Jul 18 2017, 5:53 AM
Herald added a subscriber: klimek. · View Herald TranscriptJul 18 2017, 5:53 AM
krasimir added inline comments.Jul 21 2017, 2:36 AM
lib/Format/UnwrappedLineFormatter.cpp
405

I'd change J[0] to *J and rename Line to something else so that it doesn't shadow Line.

406

Please add a test for this if.

unittests/Format/FormatTest.cpp
912

I'd suggest adding more cases here, like:

"case 6: /* comment */ x = 1; break;\n"
"case 7: x = /* comment */ 1; break;\n"
"case 8:\n"
"  x = 1; /* comment */\n"
"  break;\n"
"case 9:\n"
"  break; // comment line 1\n"
"         // comment line 2\n"
"case 10:\n"
"  return; /* comment line 1\n"
"           * comment line 2 */\n"
"}",

I'm interested also why case 10 here fails.

930

I'd like to see tests where an overly long comment line appears, like:

EXPECT_EQ("switch (a) {\n"
          "case 0:\n"
          "  return; // long long long long long long long long long long "
          "long long comment\n"
          "          // line\n"
          "}",
          format("switch (a) {\n"
                 "case 0: return; // long long long long long long long "
                 "long long long long long comment line\n"
                 "}",
                 Style));
EXPECT_EQ("switch (a) {\n"
          "case 0:\n"
          "  return; /* long long long long long long long long long long "
          "long long comment\n"
          "             line */\n"
          "}",
          format("switch (a) {\n"
                 "case 0: return; /* long long long long long long long "
                 "long long long long long comment line */\n"
                 "}",
                 Style));
Typz updated this revision to Diff 107841.Jul 23 2017, 2:31 PM
Typz marked 4 inline comments as done.

Address review comments

unittests/Format/FormatTest.cpp
912

case 10 fails because of test::messUp(), which removes the line break inside the comment.

krasimir accepted this revision.Jul 25 2017, 4:52 AM

Looks good! Thanks!

This revision is now accepted and ready to land.Jul 25 2017, 4:52 AM
Closed by commit rL309370: clang-format: merge short case labels with trailing comments (authored by Typz). · Explain WhyJul 28 2017, 12:57 AM
This revision was automatically updated to reflect the committed changes.

Revision Contents

PathSize
lib/
Format/
23 lines
unittests/
Format/
71 lines

Diff 107841

lib/Format/UnwrappedLineFormatter.cpp

Show First 20 Lines • Show All 376 Lines • ▼ Show 20 Linesprivate:
tryMergeShortCaseLabels(SmallVectorImpl<AnnotatedLine *>::const_iterator I, tryMergeShortCaseLabels(SmallVectorImpl<AnnotatedLine *>::const_iterator I,
SmallVectorImpl<AnnotatedLine *>::const_iterator E, SmallVectorImpl<AnnotatedLine *>::const_iterator E,
unsigned Limit) { unsigned Limit) {
if (Limit == 0 || I + 1 == E || if (Limit == 0 || I + 1 == E ||
I[1]->First->isOneOf(tok::kw_case, tok::kw_default)) I[1]->First->isOneOf(tok::kw_case, tok::kw_default))
return 0; return 0;
unsigned NumStmts = 0; unsigned NumStmts = 0;
unsigned Length = 0; unsigned Length = 0;
bool EndsWithComment = false;
bool InPPDirective = I[0]->InPPDirective; bool InPPDirective = I[0]->InPPDirective;
const unsigned Level = I[0]->Level;
for (; NumStmts < 3; ++NumStmts) { for (; NumStmts < 3; ++NumStmts) {
if (I + 1 + NumStmts == E) if (I + 1 + NumStmts == E)
break; break;
const AnnotatedLine *Line = I[1 + NumStmts]; const AnnotatedLine *Line = I[1 + NumStmts];
if (Line->InPPDirective != InPPDirective) if (Line->InPPDirective != InPPDirective)
break; break;
if (Line->First->isOneOf(tok::kw_case, tok::kw_default, tok::r_brace)) if (Line->First->isOneOf(tok::kw_case, tok::kw_default, tok::r_brace))
break; break;
if (Line->First->isOneOf(tok::kw_if, tok::kw_for, tok::kw_switch, if (Line->First->isOneOf(tok::kw_if, tok::kw_for, tok::kw_switch,
tok::kw_while, tok::comment) || tok::kw_while) ||
Line->Last->is(tok::comment)) EndsWithComment)
return 0; return 0;
if (Line->First->is(tok::comment)) {
if (Level != Line->Level)
return 0;
SmallVectorImpl<AnnotatedLine *>::const_iterator J = I + 2 + NumStmts;
for (; J != E; ++J) {
Line = *J;
krasimirUnsubmitted
Done
ReplyInline Actions

I'd change J[0] to *J and rename Line to something else so that it doesn't shadow Line.

krasimir: I'd change `J[0]` to `*J` and rename `Line` to something else so that it doesn't shadow `Line`.
if (Line->InPPDirective != InPPDirective)
krasimirUnsubmitted
Done
ReplyInline Actions

Please add a test for this if.

krasimir: Please add a test for this if.
break;
if (Line->First->isOneOf(tok::kw_case, tok::kw_default, tok::r_brace))
break;
if (Line->First->isNot(tok::comment) || Level != Line->Level)
return 0;
}
break;
}
if (Line->Last->is(tok::comment))
EndsWithComment = true;
Length += I[1 + NumStmts]->Last->TotalLength + 1; // 1 for the space. Length += I[1 + NumStmts]->Last->TotalLength + 1; // 1 for the space.
} }
if (NumStmts == 0 || NumStmts == 3 || Length > Limit) if (NumStmts == 0 || NumStmts == 3 || Length > Limit)
return 0; return 0;
return NumStmts; return NumStmts;
} }
unsigned unsigned
▲ Show 20 Lines • Show All 656 Lines • Show Last 20 Lines

unittests/Format/FormatTest.cpp

  • This file is larger than 256 KB, so syntax highlighting is disabled by default.
Show First 20 Lines • Show All 891 Lines • ▼ Show 20 Lines verifyFormat("switch (a) {\n"
" return;\n" " return;\n"
"case 8:\n" "case 8:\n"
" x = 8; // comment\n" " x = 8; // comment\n"
" break;\n" " break;\n"
"default: y = 1; break;\n" "default: y = 1; break;\n"
"}", "}",
Style); Style);
verifyFormat("switch (a) {\n" verifyFormat("switch (a) {\n"
"case 0: return; // comment\n"
"case 1: break; // comment\n"
"case 2: return;\n"
"// comment\n"
"case 3: return;\n"
"// comment 1\n"
"// comment 2\n"
"// comment 3\n"
"case 4: break; /* comment */\n"
"case 5:\n"
" // comment\n"
" break;\n"
"case 6: /* comment */ x = 1; break;\n"
krasimirUnsubmitted
Done
ReplyInline Actions

I'd suggest adding more cases here, like:

"case 6: /* comment */ x = 1; break;\n"
"case 7: x = /* comment */ 1; break;\n"
"case 8:\n"
"  x = 1; /* comment */\n"
"  break;\n"
"case 9:\n"
"  break; // comment line 1\n"
"         // comment line 2\n"
"case 10:\n"
"  return; /* comment line 1\n"
"           * comment line 2 */\n"
"}",

I'm interested also why case 10 here fails.

krasimir: I'd suggest adding more cases here, like: ``` "case 6: /* comment */ x = 1…
TypzAuthorUnsubmitted
Not Done
ReplyInline Actions

case 10 fails because of test::messUp(), which removes the line break inside the comment.

Typz: `case 10` fails because of test::messUp(), which removes the line break inside the comment.
"case 7: x = /* comment */ 1; break;\n"
"case 8:\n"
" x = 1; /* comment */\n"
" break;\n"
"case 9:\n"
" break; // comment line 1\n"
" // comment line 2\n"
"}",
Style);
EXPECT_EQ("switch (a) {\n"
"case 1:\n"
" x = 8;\n"
" // fall through\n"
"case 2: x = 8;\n"
"// comment\n"
"case 3:\n"
" return; /* comment line 1\n"
" * comment line 2 */\n"
krasimirUnsubmitted
Done
ReplyInline Actions

I'd like to see tests where an overly long comment line appears, like:

EXPECT_EQ("switch (a) {\n"
          "case 0:\n"
          "  return; // long long long long long long long long long long "
          "long long comment\n"
          "          // line\n"
          "}",
          format("switch (a) {\n"
                 "case 0: return; // long long long long long long long "
                 "long long long long long comment line\n"
                 "}",
                 Style));
EXPECT_EQ("switch (a) {\n"
          "case 0:\n"
          "  return; /* long long long long long long long long long long "
          "long long comment\n"
          "             line */\n"
          "}",
          format("switch (a) {\n"
                 "case 0: return; /* long long long long long long long "
                 "long long long long long comment line */\n"
                 "}",
                 Style));
krasimir: I'd like to see tests where an overly long comment line appears, like: ``` EXPECT_EQ("switch…
"case 4: i = 8;\n"
"// something else\n"
"#if FOO\n"
"case 5: break;\n"
"#endif\n"
"}",
format("switch (a) {\n"
"case 1: x = 8;\n"
" // fall through\n"
"case 2:\n"
" x = 8;\n"
"// comment\n"
"case 3:\n"
" return; /* comment line 1\n"
" * comment line 2 */\n"
"case 4:\n"
" i = 8;\n"
"// something else\n"
"#if FOO\n"
"case 5: break;\n"
"#endif\n"
"}",
Style));
EXPECT_EQ("switch (a) {\n" "case 0:\n"
" return; // long long long long long long long long long long long long comment\n"
" // line\n" "}",
format("switch (a) {\n"
"case 0: return; // long long long long long long long long long long long long comment line\n"
"}",
Style));
EXPECT_EQ("switch (a) {\n"
"case 0:\n"
" return; /* long long long long long long long long long long long long comment\n"
" line */\n"
"}",
format("switch (a) {\n"
"case 0: return; /* long long long long long long long long long long long long comment line */\n"
"}",
Style));
verifyFormat("switch (a) {\n"
"#if FOO\n" "#if FOO\n"
"case 0: return 0;\n" "case 0: return 0;\n"
"#endif\n" "#endif\n"
"}", "}",
Style); Style);
verifyFormat("switch (a) {\n" verifyFormat("switch (a) {\n"
"case 1: {\n" "case 1: {\n"
"}\n" "}\n"
▲ Show 20 Lines • Show All 10,206 Lines • Show Last 20 Lines