[PATCH] D35557: clang-format: merge short case labels with trailing comments

2017-07-28 Thread Francois Ferrand via Phabricator via cfe-commits 
This revision was automatically updated to reflect the committed changes.
Closed by commit rL309370: clang-format: merge short case labels with trailing 
comments (authored by Typz).

Repository:
  rL LLVM

https://reviews.llvm.org/D35557

Files:
  cfe/trunk/lib/Format/UnwrappedLineFormatter.cpp
  cfe/trunk/unittests/Format/FormatTest.cpp

Index: cfe/trunk/lib/Format/UnwrappedLineFormatter.cpp
===
--- cfe/trunk/lib/Format/UnwrappedLineFormatter.cpp
+++ cfe/trunk/lib/Format/UnwrappedLineFormatter.cpp
@@ -382,7 +382,9 @@
   return 0;
 unsigned NumStmts = 0;
 unsigned Length = 0;
+bool EndsWithComment = false;
 bool InPPDirective = I[0]->InPPDirective;
+const unsigned Level = I[0]->Level;
 for (; NumStmts < 3; ++NumStmts) {
   if (I + 1 + NumStmts == E)
 break;
@@ -392,9 +394,26 @@
   if (Line->First->isOneOf(tok::kw_case, tok::kw_default, tok::r_brace))
 break;
   if (Line->First->isOneOf(tok::kw_if, tok::kw_for, tok::kw_switch,
-   tok::kw_while, tok::comment) ||
-  Line->Last->is(tok::comment))
+   tok::kw_while) ||
+  EndsWithComment)
 return 0;
+  if (Line->First->is(tok::comment)) {
+if (Level != Line->Level)
+  return 0;
+SmallVectorImpl::const_iterator J = I + 2 + NumStmts;
+for (; J != E; ++J) {
+  Line = *J;
+  if (Line->InPPDirective != InPPDirective)
+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.
 }
 if (NumStmts == 0 || NumStmts == 3 || Length > Limit)
Index: cfe/trunk/unittests/Format/FormatTest.cpp
===
--- cfe/trunk/unittests/Format/FormatTest.cpp
+++ cfe/trunk/unittests/Format/FormatTest.cpp
@@ -907,6 +907,77 @@
"}",
Style);
   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"
+   "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"
+"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"
+   "}",
+

[PATCH] D35557: clang-format: merge short case labels with trailing comments

2017-07-25 Thread Krasimir Georgiev via Phabricator via cfe-commits 
krasimir accepted this revision.
krasimir added a comment.
This revision is now accepted and ready to land.

Looks good! Thanks!


https://reviews.llvm.org/D35557



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D35557: clang-format: merge short case labels with trailing comments

2017-07-23 Thread Francois Ferrand via Phabricator via cfe-commits 
Typz updated this revision to Diff 107841.
Typz marked 4 inline comments as done.
Typz added a comment.

Address review comments


https://reviews.llvm.org/D35557

Files:
  lib/Format/UnwrappedLineFormatter.cpp
  unittests/Format/FormatTest.cpp

Index: unittests/Format/FormatTest.cpp
===
--- unittests/Format/FormatTest.cpp
+++ unittests/Format/FormatTest.cpp
@@ -897,6 +897,77 @@
"}",
Style);
   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"
+   "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"
+"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"
"case 0: return 0;\n"
"#endif\n"
Index: lib/Format/UnwrappedLineFormatter.cpp
===
--- lib/Format/UnwrappedLineFormatter.cpp
+++ lib/Format/UnwrappedLineFormatter.cpp
@@ -382,7 +382,9 @@
   return 0;
 unsigned NumStmts = 0;
 unsigned Length = 0;
+bool EndsWithComment = false;
 bool InPPDirective = I[0]->InPPDirective;
+const unsigned Level = I[0]->Level;
 for (; NumStmts < 3; ++NumStmts) {
   if (I + 1 + NumStmts == E)
 break;
@@ -392,9 +394,26 @@
   if (Line->First->isOneOf(tok::kw_case, tok::kw_default, tok::r_brace))
 break;
   if (Line->First->isOneOf(tok::kw_if, tok::kw_for, tok::kw_switch,
-   tok::kw_while, tok::comment) ||
-  Line->Last->is(tok::comment))
+   tok::kw_while) ||
+  EndsWithComment)
 return 0;
+  if (Line->First->is(tok::comment)) {
+if (Level != Line->Level)
+  return 0;
+SmallVectorImpl::const_iterator J = I + 2 + NumStmts;
+for (; J != E; ++J) {
+  Line = *J;
+  if (Line->InPPDirective != InPPDirective)
+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.
 }
 if (NumStmts == 0 || NumStmts == 3 || Length > Limit)

[PATCH] D35557: clang-format: merge short case labels with trailing comments

2017-07-23 Thread Francois Ferrand via Phabricator via cfe-commits 
Typz added inline comments.



Comment at: unittests/Format/FormatTest.cpp:912
+   "  break;\n"
+   "}",
+   Style);

krasimir wrote:
> 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.
`case 10` fails because of test::messUp(), which removes the line break inside 
the comment.


https://reviews.llvm.org/D35557



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D35557: clang-format: merge short case labels with trailing comments

2017-07-21 Thread Krasimir Georgiev via Phabricator via cfe-commits 
krasimir added inline comments.



Comment at: lib/Format/UnwrappedLineFormatter.cpp:405
+for (; J != E; ++J) {
+  const AnnotatedLine *Line = J[0];
+  if (Line->InPPDirective != InPPDirective)

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



Comment at: lib/Format/UnwrappedLineFormatter.cpp:406
+  const AnnotatedLine *Line = J[0];
+  if (Line->InPPDirective != InPPDirective)
+break;

Please add a test for this if.



Comment at: unittests/Format/FormatTest.cpp:912
+   "  break;\n"
+   "}",
+   Style);

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.



Comment at: unittests/Format/FormatTest.cpp:930
+   "}",
+   Style));
+  verifyFormat("switch (a) {\n"

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));
```


https://reviews.llvm.org/D35557



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D35557: clang-format: merge short case labels with trailing comments

2017-07-18 Thread Francois Ferrand via Phabricator via cfe-commits 
Typz created this revision.
Herald added a subscriber: klimek.

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:
  }


https://reviews.llvm.org/D35557

Files:
  lib/Format/UnwrappedLineFormatter.cpp
  unittests/Format/FormatTest.cpp


Index: unittests/Format/FormatTest.cpp
===
--- unittests/Format/FormatTest.cpp
+++ unittests/Format/FormatTest.cpp
@@ -897,6 +897,38 @@
"}",
Style);
   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"
+   "}",
+   Style);
+  EXPECT_EQ("switch (a) {\n"
+"case 1:\n"
+"  x = 8;\n"
+"  // fall through\n"
+"case 2: x = 8;\n"
+"// comment\n"
+"default:\n"
+"}",
+format("switch (a) {\n"
+   "case 1: x = 8;\n"
+   "  // fall through\n"
+   "case 2:\n"
+   "  x = 8;\n"
+   "// comment\n"
+   "default:\n"
+   "}",
+   Style));
+  verifyFormat("switch (a) {\n"
"#if FOO\n"
"case 0: return 0;\n"
"#endif\n"
Index: lib/Format/UnwrappedLineFormatter.cpp
===
--- lib/Format/UnwrappedLineFormatter.cpp
+++ lib/Format/UnwrappedLineFormatter.cpp
@@ -382,7 +382,9 @@
   return 0;
 unsigned NumStmts = 0;
 unsigned Length = 0;
+bool EndsWithComment = false;
 bool InPPDirective = I[0]->InPPDirective;
+const unsigned Level = I[0]->Level;
 for (; NumStmts < 3; ++NumStmts) {
   if (I + 1 + NumStmts == E)
 break;
@@ -392,9 +394,26 @@
   if (Line->First->isOneOf(tok::kw_case, tok::kw_default, tok::r_brace))
 break;
   if (Line->First->isOneOf(tok::kw_if, tok::kw_for, tok::kw_switch,
-   tok::kw_while, tok::comment) ||
-  Line->Last->is(tok::comment))
+   tok::kw_while) ||
+  EndsWithComment)
 return 0;
+  if (Line->First->is(tok::comment)) {
+if (Level != Line->Level)
+  return 0;
+SmallVectorImpl::const_iterator J = I + 2 + NumStmts;
+for (; J != E; ++J) {
+  const AnnotatedLine *Line = J[0];
+  if (Line->InPPDirective != InPPDirective)
+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.
 }
 if (NumStmts == 0 || NumStmts == 3 || Length > Limit)


Index: unittests/Format/FormatTest.cpp
===
--- unittests/Format/FormatTest.cpp
+++ unittests/Format/FormatTest.cpp
@@ -897,6 +897,38 @@
"}",
Style);
   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"
+   "}",
+   Style);
+  EXPECT_EQ("switch (a) {\n"
+"case 1:\n"
+"  x = 8;\n"
+"  // fall through\n"
+"case 2: x = 8;\n"
+"// comment\n"
+"default:\n"
+"}",
+format("switch (a) {\n"
+   "case 1: x = 8;\n"
+   "  // fall through\n"
+   "case 2:\n"
+   "  x = 8;\n"
+   "// comment\n"
+   "default:\n"
+   "}",
+   Style));
+  verifyFormat("switch (a) {\n"
"#if FOO\n"
"case 0: return 0;\n"