Page MenuHomePhabricator

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

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

Details

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

Repository
rL LLVM

Event Timeline

Typz created this revision.Jul 18 2017, 5:53 AM
krasimir added inline comments.Jul 21 2017, 2:36 AM
lib/Format/UnwrappedLineFormatter.cpp
405 ↗(On Diff #107085)

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

406 ↗(On Diff #107085)

Please add a test for this if.

unittests/Format/FormatTest.cpp
912 ↗(On Diff #107085)

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 ↗(On Diff #107085)

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 ↗(On Diff #107085)

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
This revision was automatically updated to reflect the committed changes.