[PATCH] D151761: clang-format: Add AlignConsecutiveShortCaseStatements

[Owen Pan via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG4ba00844174d: [clang-format] Add 
AlignConsecutiveShortCaseStatements (authored by owenpan).

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D151761/new/

https://reviews.llvm.org/D151761

Files:
  clang/docs/ClangFormatStyleOptions.rst
  clang/docs/ReleaseNotes.rst
  clang/include/clang/Format/Format.h
  clang/lib/Format/Format.cpp
  clang/lib/Format/WhitespaceManager.cpp
  clang/lib/Format/WhitespaceManager.h
  clang/unittests/Format/ConfigParseTest.cpp
  clang/unittests/Format/FormatTest.cpp

Index: clang/unittests/Format/FormatTest.cpp
===
--- clang/unittests/Format/FormatTest.cpp
+++ clang/unittests/Format/FormatTest.cpp
@@ -19261,6 +19261,240 @@
BracedAlign);
 }
 
+TEST_F(FormatTest, AlignConsecutiveShortCaseStatements) {
+  FormatStyle Alignment = getLLVMStyle();
+  Alignment.AllowShortCaseLabelsOnASingleLine = true;
+  Alignment.AlignConsecutiveShortCaseStatements.Enabled = true;
+
+  verifyFormat("switch (level) {\n"
+   "case log::info:return \"info\";\n"
+   "case log::warning: return \"warning\";\n"
+   "default:   return \"default\";\n"
+   "}",
+   Alignment);
+
+  verifyFormat("switch (level) {\n"
+   "case log::info:return \"info\";\n"
+   "case log::warning: return \"warning\";\n"
+   "}",
+   "switch (level) {\n"
+   "case log::info: return \"info\";\n"
+   "case log::warning:\n"
+   "  return \"warning\";\n"
+   "}",
+   Alignment);
+
+  // Empty case statements push out the alignment, but non-short case labels
+  // don't.
+  verifyFormat("switch (level) {\n"
+   "case log::info: return \"info\";\n"
+   "case log::critical:\n"
+   "case log::warning:\n"
+   "case log::severe:   return \"severe\";\n"
+   "case log::extra_severe:\n"
+   "  // comment\n"
+   "  return \"extra_severe\";\n"
+   "}",
+   Alignment);
+
+  // Verify comments and empty lines break the alignment.
+  verifyNoChange("switch (level) {\n"
+ "case log::info:return \"info\";\n"
+ "case log::warning: return \"warning\";\n"
+ "// comment\n"
+ "case log::critical: return \"critical\";\n"
+ "default:return \"default\";\n"
+ "\n"
+ "case log::severe: return \"severe\";\n"
+ "}",
+ Alignment);
+
+  // Empty case statements don't break the alignment, and potentially push it
+  // out.
+  verifyFormat("switch (level) {\n"
+   "case log::info: return \"info\";\n"
+   "case log::warning:\n"
+   "case log::critical:\n"
+   "default:return \"default\";\n"
+   "}",
+   Alignment);
+
+  // Implicit fallthrough cases can be aligned with either a comment or
+  // [[fallthrough]]
+  verifyFormat("switch (level) {\n"
+   "case log::info: return \"info\";\n"
+   "case log::warning:  // fallthrough\n"
+   "case log::error:return \"error\";\n"
+   "case log::critical: /*fallthrough*/\n"
+   "case log::severe:   return \"severe\";\n"
+   "case log::diag: [[fallthrough]];\n"
+   "default:return \"default\";\n"
+   "}",
+   Alignment);
+
+  // Verify trailing comment that needs a reflow also gets aligned properly.
+  verifyFormat("switch (level) {\n"
+   "case log::info:return \"info\";\n"
+   "case log::warning: // fallthrough\n"
+   "case log::error:   return \"error\";\n"
+   "}",
+   "switch (level) {\n"
+   "case log::info:return \"info\";\n"
+   "case log::warning: //fallthrough\n"
+   "case log::error:   return \"error\";\n"
+   "}",
+   Alignment);
+
+  // Verify adjacent non-short case statements don't change the alignment, and
+  // properly break the set of consecutive statements.
+  verifyFormat("switch (level) {\n"
+   "case log::critical:\n"
+   "  // comment\n"
+   "  return \"critical\";\n"
+   "case log::info:return \"info\";\n"
+   "case log::warning: return \"warning\";\n"
+   "default:\n"
+   "  // comment\n"
+   "  return \"\";\n"
+   "case log::error:  return \"error\";\n"
+   "case log::severe: return \"severe\";\n"
+   "case log::extra_critical:\n"
+   "  // comment\n"
+ 

[PATCH] D151761: clang-format: Add AlignConsecutiveShortCaseStatements

[Björn Schäpers via Phabricator via cfe-commits]

HazardyKnusperkeks accepted this revision.
HazardyKnusperkeks added a comment.

All of those variants are fine by me, but I'd stick with the current version, 
because it is linked to `AllowShortCaseLabelsOnASingleLine`.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D151761/new/

https://reviews.llvm.org/D151761

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

[PATCH] D151761: clang-format: Add AlignConsecutiveShortCaseStatements

[Owen Pan via Phabricator via cfe-commits]

owenpan accepted this revision.
owenpan added a comment.

In D151761#4528389 , @galenelias 
wrote:

> In D151761#4524653 , @owenpan wrote:
>
>> FWIW, I think we can use a shorter name `AlignConsecutiveCaseStatements` 
>> instead of `AlignConsecutiveShortCaseStatements`.
>
> My only hesitation with that name is that it might seem like something like 
> there should be some alignment being applied to 'normal' consecutive case 
> statements, which there isn't.  Maybe it's fine because the documentation 
> makes it clear?  I'm definitely not picky about the name, whatever sounds 
> idiomatic.  @HazardyKnusperkeks, thoughts on just 
> `AlignConsecutiveCaseStatements`?
>
>   switch (level) {
>   case 0:
>   case 100:
>  return "error";
>   }

Strictly speaking, `case 0:` in the example above is an empty `case` statement, 
so if we really want to be precise and verbose, we should use 
`AlignConsecutiveNonEmptyCaseStatements` or even 
`AlignConsecutiveNonEmptyShortCaseStatements`? I still prefer the shorter 
`AlignConsecutiveCaseStatements`, but will leave it to others.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D151761/new/

https://reviews.llvm.org/D151761

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

[PATCH] D151761: clang-format: Add AlignConsecutiveShortCaseStatements

[Galen Elias via Phabricator via cfe-commits]

galenelias updated this revision to Diff 543558.
galenelias added a comment.

Addresses latest review comments.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D151761/new/

https://reviews.llvm.org/D151761

Files:
  clang/docs/ClangFormatStyleOptions.rst
  clang/docs/ReleaseNotes.rst
  clang/include/clang/Format/Format.h
  clang/lib/Format/Format.cpp
  clang/lib/Format/WhitespaceManager.cpp
  clang/lib/Format/WhitespaceManager.h
  clang/unittests/Format/ConfigParseTest.cpp
  clang/unittests/Format/FormatTest.cpp

Index: clang/unittests/Format/FormatTest.cpp
===
--- clang/unittests/Format/FormatTest.cpp
+++ clang/unittests/Format/FormatTest.cpp
@@ -19261,6 +19261,240 @@
BracedAlign);
 }
 
+TEST_F(FormatTest, AlignConsecutiveShortCaseStatements) {
+  FormatStyle Alignment = getLLVMStyle();
+  Alignment.AllowShortCaseLabelsOnASingleLine = true;
+  Alignment.AlignConsecutiveShortCaseStatements.Enabled = true;
+
+  verifyFormat("switch (level) {\n"
+   "case log::info:return \"info\";\n"
+   "case log::warning: return \"warning\";\n"
+   "default:   return \"default\";\n"
+   "}",
+   Alignment);
+
+  verifyFormat("switch (level) {\n"
+   "case log::info:return \"info\";\n"
+   "case log::warning: return \"warning\";\n"
+   "}",
+   "switch (level) {\n"
+   "case log::info: return \"info\";\n"
+   "case log::warning:\n"
+   "  return \"warning\";\n"
+   "}",
+   Alignment);
+
+  // Empty case statements push out the alignment, but non-short case labels
+  // don't.
+  verifyFormat("switch (level) {\n"
+   "case log::info: return \"info\";\n"
+   "case log::critical:\n"
+   "case log::warning:\n"
+   "case log::severe:   return \"severe\";\n"
+   "case log::extra_severe:\n"
+   "  // comment\n"
+   "  return \"extra_severe\";\n"
+   "}",
+   Alignment);
+
+  // Verify comments and empty lines break the alignment.
+  verifyNoChange("switch (level) {\n"
+ "case log::info:return \"info\";\n"
+ "case log::warning: return \"warning\";\n"
+ "// comment\n"
+ "case log::critical: return \"critical\";\n"
+ "default:return \"default\";\n"
+ "\n"
+ "case log::severe: return \"severe\";\n"
+ "}",
+ Alignment);
+
+  // Empty case statements don't break the alignment, and potentially push it
+  // out.
+  verifyFormat("switch (level) {\n"
+   "case log::info: return \"info\";\n"
+   "case log::warning:\n"
+   "case log::critical:\n"
+   "default:return \"default\";\n"
+   "}",
+   Alignment);
+
+  // Implicit fallthrough cases can be aligned with either a comment or
+  // [[fallthrough]]
+  verifyFormat("switch (level) {\n"
+   "case log::info: return \"info\";\n"
+   "case log::warning:  // fallthrough\n"
+   "case log::error:return \"error\";\n"
+   "case log::critical: /*fallthrough*/\n"
+   "case log::severe:   return \"severe\";\n"
+   "case log::diag: [[fallthrough]];\n"
+   "default:return \"default\";\n"
+   "}",
+   Alignment);
+
+  // Verify trailing comment that needs a reflow also gets aligned properly.
+  verifyFormat("switch (level) {\n"
+   "case log::info:return \"info\";\n"
+   "case log::warning: // fallthrough\n"
+   "case log::error:   return \"error\";\n"
+   "}",
+   "switch (level) {\n"
+   "case log::info:return \"info\";\n"
+   "case log::warning: //fallthrough\n"
+   "case log::error:   return \"error\";\n"
+   "}",
+   Alignment);
+
+  // Verify adjacent non-short case statements don't change the alignment, and
+  // properly break the set of consecutive statements.
+  verifyFormat("switch (level) {\n"
+   "case log::critical:\n"
+   "  // comment\n"
+   "  return \"critical\";\n"
+   "case log::info:return \"info\";\n"
+   "case log::warning: return \"warning\";\n"
+   "default:\n"
+   "  // comment\n"
+   "  return \"\";\n"
+   "case log::error:  return \"error\";\n"
+   "case log::severe: return \"severe\";\n"
+   "case log::extra_critical:\n"
+   "  // comment\n"
+   "  return \"extra critical\";\n"
+   "}",
+   Alignment);
+
+  

[PATCH] D151761: clang-format: Add AlignConsecutiveShortCaseStatements

[Galen Elias via Phabricator via cfe-commits]

galenelias marked 6 inline comments as done.
galenelias added a comment.

In D151761#4524653 , @owenpan wrote:

> FWIW, I think we can use a shorter name `AlignConsecutiveCaseStatements` 
> instead of `AlignConsecutiveShortCaseStatements`.

My only hesitation with that name is that it might seem like something like 
there should be some alignment being applied to 'normal' consecutive case 
statements, which there isn't.  Maybe it's fine because the documentation makes 
it clear?  I'm definitely not picky about the name, whatever sounds idiomatic.  
@HazardyKnusperkeks, thoughts on just `AlignConsecutiveCaseStatements`?

  switch (level) {
  case 0:
  case 100:
 return "error";
  }


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D151761/new/

https://reviews.llvm.org/D151761

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

[PATCH] D151761: clang-format: Add AlignConsecutiveShortCaseStatements

[Owen Pan via Phabricator via cfe-commits]

owenpan added a comment.

FWIW, I think we can use a shorter name `AlignConsecutiveCaseStatements` 
instead of `AlignConsecutiveShortCaseStatements`.




Comment at: clang/lib/Format/WhitespaceManager.cpp:854
+  return C.Tok->is(TT_CaseLabelColon);
+} else {
+  // Ignore 'IsInsideToken' to allow matching trailing comments which

No `else` after `return`.



Comment at: clang/lib/Format/WhitespaceManager.cpp:859-860
+  // reflow early due to detecting multiple aligning tokens per line.
+  return (!C.IsInsideToken && C.Tok->Previous &&
+  C.Tok->Previous->is(TT_CaseLabelColon));
+}

Please remove the redundant parentheses.



Comment at: clang/lib/Format/WhitespaceManager.cpp:909
+
+if (!Changes[I].Tok->is(tok::comment))
+  LineIsComment = false;





Comment at: clang/lib/Format/WhitespaceManager.cpp:913
+if (Changes[I].Tok->is(TT_CaseLabelColon)) {
+  LineIsEmptyCase = Changes[I].Tok->Next == nullptr ||
+Changes[I].Tok->Next->isTrailingComment();





Comment at: clang/unittests/Format/FormatTest.cpp:19284-19293
+  verifyFormat("switch (level) {\n"
+   "case log::info:return \"info\";\n"
+   "case log::warning: return \"warning\";\n"
+   "// comment\n"
+   "case log::critical: return \"critical\";\n"
+   "default:return \"default\";\n"
+   "\n"





Comment at: clang/unittests/Format/FormatTest.cpp:19218
+  // Verify comments and empty lines break the alignment
+  verifyFormat("switch (level) {\n"
+   "case log::info:return \"info\";\n"

HazardyKnusperkeks wrote:
> galenelias wrote:
> > HazardyKnusperkeks wrote:
> > > If both strings are the same, you only need to supply it once.
> > > 
> > > Or are they different and I can't see it?
> > They are the same, but I can't use the single string version, as that one 
> > calls test::messUp on the string which will strip blank lines, which then 
> > invalidates the test case.  It seems pretty much all tests which are trying 
> > to validate behavior around empty spaces has to use the two string version.
> Check.
> They are the same, but I can't use the single string version, as that one 
> calls test::messUp on the string which will strip blank lines, which then 
> invalidates the test case.  It seems pretty much all tests which are trying 
> to validate behavior around empty spaces has to use the two string version.

We have `verifyNoChange` for that now.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D151761/new/

https://reviews.llvm.org/D151761

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

[PATCH] D151761: clang-format: Add AlignConsecutiveShortCaseStatements

[Galen Elias via Phabricator via cfe-commits]

galenelias added a comment.

In D151761#4483050 , 
@HazardyKnusperkeks wrote:

> Thanks for the patience, I'm really looking forward to use this.
>
> But please wait for other opinions.

Thanks so much @HazardyKnusperkeks for all the feedback and help.  Out of 
curiosity, how long is typical to wait for additional opinions?  I just want to 
make sure this doesn't get lost.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D151761/new/

https://reviews.llvm.org/D151761

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

[PATCH] D151761: clang-format: Add AlignConsecutiveShortCaseStatements

[Björn Schäpers via Phabricator via cfe-commits]

HazardyKnusperkeks accepted this revision.
HazardyKnusperkeks added a comment.
This revision is now accepted and ready to land.

Thanks for the patience, I'm really looking forward to use this.

But please wait for other opinions.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D151761/new/

https://reviews.llvm.org/D151761

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

[PATCH] D151761: clang-format: Add AlignConsecutiveShortCaseStatements

[Galen Elias via Phabricator via cfe-commits]

galenelias updated this revision to Diff 538288.
galenelias marked 2 inline comments as done.
galenelias added a comment.

Addressed outstanding review comments.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D151761/new/

https://reviews.llvm.org/D151761

Files:
  clang/docs/ClangFormatStyleOptions.rst
  clang/docs/ReleaseNotes.rst
  clang/include/clang/Format/Format.h
  clang/lib/Format/Format.cpp
  clang/lib/Format/WhitespaceManager.cpp
  clang/lib/Format/WhitespaceManager.h
  clang/unittests/Format/ConfigParseTest.cpp
  clang/unittests/Format/FormatTest.cpp

Index: clang/unittests/Format/FormatTest.cpp
===
--- clang/unittests/Format/FormatTest.cpp
+++ clang/unittests/Format/FormatTest.cpp
@@ -19244,6 +19244,257 @@
BracedAlign);
 }
 
+TEST_F(FormatTest, AlignConsecutiveShortCaseStatements) {
+  FormatStyle Alignment = getLLVMStyle();
+  Alignment.AllowShortCaseLabelsOnASingleLine = true;
+  Alignment.AlignConsecutiveShortCaseStatements.Enabled = true;
+
+  verifyFormat("switch (level) {\n"
+   "case log::info:return \"info\";\n"
+   "case log::warning: return \"warning\";\n"
+   "default:   return \"default\";\n"
+   "}",
+   Alignment);
+
+  verifyFormat("switch (level) {\n"
+   "case log::info:return \"info\";\n"
+   "case log::warning: return \"warning\";\n"
+   "}",
+   "switch (level) {\n"
+   "case log::info: return \"info\";\n"
+   "case log::warning:\n"
+   "  return \"warning\";\n"
+   "}",
+   Alignment);
+
+  // Empty case statements push out the alignment, but non-short case labels
+  // don't.
+  verifyFormat("switch (level) {\n"
+   "case log::info: return \"info\";\n"
+   "case log::critical:\n"
+   "case log::warning:\n"
+   "case log::severe:   return \"severe\";\n"
+   "case log::extra_severe:\n"
+   "  // comment\n"
+   "  return \"extra_severe\";\n"
+   "}",
+   Alignment);
+
+  // Verify comments and empty lines break the alignment.
+  verifyFormat("switch (level) {\n"
+   "case log::info:return \"info\";\n"
+   "case log::warning: return \"warning\";\n"
+   "// comment\n"
+   "case log::critical: return \"critical\";\n"
+   "default:return \"default\";\n"
+   "\n"
+   "case log::severe: return \"severe\";\n"
+   "}",
+   "switch (level) {\n"
+   "case log::info:return \"info\";\n"
+   "case log::warning: return \"warning\";\n"
+   "// comment\n"
+   "case log::critical: return \"critical\";\n"
+   "default:return \"default\";\n"
+   "\n"
+   "case log::severe: return \"severe\";\n"
+   "}",
+   Alignment);
+
+  // Empty case statements don't break the alignment, and potentially push it
+  // out.
+  verifyFormat("switch (level) {\n"
+   "case log::info: return \"info\";\n"
+   "case log::warning:\n"
+   "case log::critical:\n"
+   "default:return \"default\";\n"
+   "}",
+   Alignment);
+
+  // Implicit fallthrough cases can be aligned with either a comment or
+  // [[fallthrough]]
+  verifyFormat("switch (level) {\n"
+   "case log::info: return \"info\";\n"
+   "case log::warning:  // fallthrough\n"
+   "case log::error:return \"error\";\n"
+   "case log::critical: /*fallthrough*/\n"
+   "case log::severe:   return \"severe\";\n"
+   "case log::diag: [[fallthrough]];\n"
+   "default:return \"default\";\n"
+   "}",
+   Alignment);
+
+  // Verify trailing comment that needs a reflow also gets aligned properly.
+  verifyFormat("switch (level) {\n"
+   "case log::info:return \"info\";\n"
+   "case log::warning: // fallthrough\n"
+   "case log::error:   return \"error\";\n"
+   "}",
+   "switch (level) {\n"
+   "case log::info:return \"info\";\n"
+   "case log::warning: //fallthrough\n"
+   "case log::error:   return \"error\";\n"
+   "}",
+   Alignment);
+
+  // Verify adjacent non-short case statements don't change the alignment, and
+  // properly break the set of consecutive statements.
+  verifyFormat("switch (level) {\n"
+   "case log::critical:\n"
+   "  // comment\n"
+   "  return \"critical\";\n"
+   "case log::info:return \"info\";\n"
+   "case 

[PATCH] D151761: clang-format: Add AlignConsecutiveShortCaseStatements

[Galen Elias via Phabricator via cfe-commits]

galenelias marked 6 inline comments as done.
galenelias added inline comments.



Comment at: clang/include/clang/Format/Format.h:380
+}
+bool operator!=(const ShortCaseStatementsAlignmentStyle ) const {
+  return !(*this == R);

HazardyKnusperkeks wrote:
> I'd drop that. We don't have it for any other struct.
> 
> And with C++20 you don't need this anymore (although I don't know when llvm 
> will switch to c++20).
We do have it for `TrailingCommentsAlignmentStyle` and `AlignConsecutiveStyle`, 
but I will drop it.



Comment at: clang/unittests/Format/ConfigParseTest.cpp:321
   CHECK_ALIGN_CONSECUTIVE(AlignConsecutiveDeclarations);
+  CHECK_ALIGN_CONSECUTIVE(AlignConsecutiveShortCaseStatements);
 

HazardyKnusperkeks wrote:
> You now have to write your own checks for the parsing. (It's just copy & 
> paste.)
Aah, I had missed the `CHECK_PARSE_NESTED_BOOL`s for the existing 
alignConsecutive options within their macro.  Will fix. Thanks


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D151761/new/

https://reviews.llvm.org/D151761

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

[PATCH] D151761: clang-format: Add AlignConsecutiveShortCaseStatements

[Björn Schäpers via Phabricator via cfe-commits]

HazardyKnusperkeks added inline comments.



Comment at: clang/include/clang/Format/Format.h:380
+}
+bool operator!=(const ShortCaseStatementsAlignmentStyle ) const {
+  return !(*this == R);

I'd drop that. We don't have it for any other struct.

And with C++20 you don't need this anymore (although I don't know when llvm 
will switch to c++20).



Comment at: clang/lib/Format/WhitespaceManager.cpp:108
   alignConsecutiveAssignments();
+  alignConsecutiveShortCaseStatements();
   alignChainedConditionals();

galenelias wrote:
> HazardyKnusperkeks wrote:
> > Is this the right position?
> > Could you add a test with aligning assignments and case statements?
> > 
> > ```
> > case XX: xx = 2; break;
> > case X: x = 1; break;
> > ```
> > It should end up as
> > ```
> > case XX: xx = 2; break;
> > case X:  x  = 1; break;
> > ``` 
> Great point, I hadn't actually really fully considered the ordering 
> dependency here.  I agree that aligning short case statements before 
> assignments (and probably declarations?) seems more correct.  However, these 
> other alignConsecutive* methods don't actually align across scopes, so won't 
> align declarations/assignments within the body of a case statement.  I 
> verified that these don't align today using AlignConsecutiveAssignments.  I 
> will however move this up to try to make it as logically correct as possible.
Too bad, that would really be nice. :)
But certainly out of scope for this change.



Comment at: clang/unittests/Format/ConfigParseTest.cpp:321
   CHECK_ALIGN_CONSECUTIVE(AlignConsecutiveDeclarations);
+  CHECK_ALIGN_CONSECUTIVE(AlignConsecutiveShortCaseStatements);
 

You now have to write your own checks for the parsing. (It's just copy & paste.)


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D151761/new/

https://reviews.llvm.org/D151761

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

[PATCH] D151761: clang-format: Add AlignConsecutiveShortCaseStatements

[Galen Elias via Phabricator via cfe-commits]

galenelias updated this revision to Diff 538228.
galenelias marked an inline comment as done.
galenelias edited the summary of this revision.
galenelias added a comment.

This iteration switches away from using `AlignConsecutiveStyle` and instead 
uses a new `ShortCaseStatementsAlignmentStyle`.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D151761/new/

https://reviews.llvm.org/D151761

Files:
  clang/docs/ClangFormatStyleOptions.rst
  clang/docs/ReleaseNotes.rst
  clang/include/clang/Format/Format.h
  clang/lib/Format/Format.cpp
  clang/lib/Format/WhitespaceManager.cpp
  clang/lib/Format/WhitespaceManager.h
  clang/unittests/Format/FormatTest.cpp

Index: clang/unittests/Format/FormatTest.cpp
===
--- clang/unittests/Format/FormatTest.cpp
+++ clang/unittests/Format/FormatTest.cpp
@@ -19244,6 +19244,257 @@
BracedAlign);
 }
 
+TEST_F(FormatTest, AlignConsecutiveShortCaseStatements) {
+  FormatStyle Alignment = getLLVMStyle();
+  Alignment.AllowShortCaseLabelsOnASingleLine = true;
+  Alignment.AlignConsecutiveShortCaseStatements.Enabled = true;
+
+  verifyFormat("switch (level) {\n"
+   "case log::info:return \"info\";\n"
+   "case log::warning: return \"warning\";\n"
+   "default:   return \"default\";\n"
+   "}",
+   Alignment);
+
+  verifyFormat("switch (level) {\n"
+   "case log::info:return \"info\";\n"
+   "case log::warning: return \"warning\";\n"
+   "}",
+   "switch (level) {\n"
+   "case log::info: return \"info\";\n"
+   "case log::warning:\n"
+   "  return \"warning\";\n"
+   "}",
+   Alignment);
+
+  // Empty case statements push out the alignment, but non-short case labels
+  // don't.
+  verifyFormat("switch (level) {\n"
+   "case log::info: return \"info\";\n"
+   "case log::critical:\n"
+   "case log::warning:\n"
+   "case log::severe:   return \"severe\";\n"
+   "case log::extra_severe:\n"
+   "  // comment\n"
+   "  return \"extra_severe\";\n"
+   "}",
+   Alignment);
+
+  // Verify comments and empty lines break the alignment.
+  verifyFormat("switch (level) {\n"
+   "case log::info:return \"info\";\n"
+   "case log::warning: return \"warning\";\n"
+   "// comment\n"
+   "case log::critical: return \"critical\";\n"
+   "default:return \"default\";\n"
+   "\n"
+   "case log::severe: return \"severe\";\n"
+   "}",
+   "switch (level) {\n"
+   "case log::info:return \"info\";\n"
+   "case log::warning: return \"warning\";\n"
+   "// comment\n"
+   "case log::critical: return \"critical\";\n"
+   "default:return \"default\";\n"
+   "\n"
+   "case log::severe: return \"severe\";\n"
+   "}",
+   Alignment);
+
+  // Empty case statements don't break the alignment, and potentially push it
+  // out.
+  verifyFormat("switch (level) {\n"
+   "case log::info: return \"info\";\n"
+   "case log::warning:\n"
+   "case log::critical:\n"
+   "default:return \"default\";\n"
+   "}",
+   Alignment);
+
+  // Implicit fallthrough cases can be aligned with either a comment or
+  // [[fallthrough]]
+  verifyFormat("switch (level) {\n"
+   "case log::info: return \"info\";\n"
+   "case log::warning:  // fallthrough\n"
+   "case log::error:return \"error\";\n"
+   "case log::critical: /*fallthrough*/\n"
+   "case log::severe:   return \"severe\";\n"
+   "case log::diag: [[fallthrough]];\n"
+   "default:return \"default\";\n"
+   "}",
+   Alignment);
+
+  // Verify trailing comment that needs a reflow also gets aligned properly.
+  verifyFormat("switch (level) {\n"
+   "case log::info:return \"info\";\n"
+   "case log::warning: // fallthrough\n"
+   "case log::error:   return \"error\";\n"
+   "}",
+   "switch (level) {\n"
+   "case log::info:return \"info\";\n"
+   "case log::warning: //fallthrough\n"
+   "case log::error:   return \"error\";\n"
+   "}",
+   Alignment);
+
+  // Verify adjacent non-short case statements don't change the alignment, and
+  // properly break the set of consecutive statements.
+  verifyFormat("switch (level) {\n"
+   "case log::critical:\n"
+   "  // comment\n"
+   "  return 

[PATCH] D151761: clang-format: Add AlignConsecutiveShortCaseStatements

[Galen Elias via Phabricator via cfe-commits]

galenelias marked an inline comment as done.
galenelias added inline comments.



Comment at: clang/include/clang/Format/Format.h:327
+  /// \version 17
+  AlignConsecutiveStyle AlignConsecutiveShortCaseStatements;
 

HazardyKnusperkeks wrote:
> Since you are not using AlignTokens anymore, I'd say use your own struct. You 
> don't have the not used memberand you don't have to add a new member which is 
> not used by AlignTokens.
Sure. Will post an iteration with this - hopefully I'm getting all the plumbing 
correct.



Comment at: clang/lib/Format/WhitespaceManager.cpp:108
   alignConsecutiveAssignments();
+  alignConsecutiveShortCaseStatements();
   alignChainedConditionals();

HazardyKnusperkeks wrote:
> Is this the right position?
> Could you add a test with aligning assignments and case statements?
> 
> ```
> case XX: xx = 2; break;
> case X: x = 1; break;
> ```
> It should end up as
> ```
> case XX: xx = 2; break;
> case X:  x  = 1; break;
> ``` 
Great point, I hadn't actually really fully considered the ordering dependency 
here.  I agree that aligning short case statements before assignments (and 
probably declarations?) seems more correct.  However, these other 
alignConsecutive* methods don't actually align across scopes, so won't align 
declarations/assignments within the body of a case statement.  I verified that 
these don't align today using AlignConsecutiveAssignments.  I will however move 
this up to try to make it as logically correct as possible.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D151761/new/

https://reviews.llvm.org/D151761

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

[PATCH] D151761: clang-format: Add AlignConsecutiveShortCaseStatements

[Björn Schäpers via Phabricator via cfe-commits]

HazardyKnusperkeks added a comment.

In D151761#4474136 , @galenelias 
wrote:

> I re-wrote the alignment to stop using AlignTokens so that I can now handle 
> all the edge cases that came up.  Specifically:
>
> - Allowing empty case labels (implicit fall through) to not break the 
> alignment, but only if they are sandwiched by short case statements.
> - Don't align the colon of a non-short case label that follows short case 
> labels when using 'AlignCaseColons=true'.
> - Empty case labels will also now push out the alignment of the statements 
> when using AlignCaseColons=false.
>
> Also, this now avoids having to add the `IgnoreNestedScopes` parameter to 
> `AlignTokens` which didn't feel great.
>
> I refactored `AlignMacroSequence` so I could re-use the core aligning method, 
> since it's the same logic I need, but I removed some of the dead code just to 
> simplify things along the way.  But even with that, this version is quite a 
> bit more code (~100 lines vs. ~30 lines).

Nice work!




Comment at: clang/include/clang/Format/Format.h:327
+  /// \version 17
+  AlignConsecutiveStyle AlignConsecutiveShortCaseStatements;
 

Since you are not using AlignTokens anymore, I'd say use your own struct. You 
don't have the not used memberand you don't have to add a new member which is 
not used by AlignTokens.



Comment at: clang/lib/Format/WhitespaceManager.cpp:108
   alignConsecutiveAssignments();
+  alignConsecutiveShortCaseStatements();
   alignChainedConditionals();

Is this the right position?
Could you add a test with aligning assignments and case statements?

```
case XX: xx = 2; break;
case X: x = 1; break;
```
It should end up as
```
case XX: xx = 2; break;
case X:  x  = 1; break;
``` 


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D151761/new/

https://reviews.llvm.org/D151761

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

[PATCH] D151761: clang-format: Add AlignConsecutiveShortCaseStatements

[Galen Elias via Phabricator via cfe-commits]

galenelias updated this revision to Diff 537383.
galenelias edited the summary of this revision.
galenelias added a comment.

I re-wrote the alignment to stop using AlignTokens so that I can now handle all 
the edge cases that came up.  Specifically:

- Allowing empty case labels (implicit fall through) to not break the 
alignment, but only if they are sandwiched by short case statements.
- Don't align the colon of a non-short case label that follows short case 
labels when using 'AlignCaseColons=true'.
- Empty case labels will also now push out the alignment of the statements when 
using AlignCaseColons=false.

Also, this now avoids having to add the `IgnoreNestedScopes` parameter to 
`AlignTokens` which didn't feel great.

I refactored `AlignMacroSequence` so I could re-use the core aligning method, 
since it's the same logic I need, but I removed some of the dead code just to 
simplify things along the way.  But even with that, this version is quite a bit 
more code (~100 lines vs. ~30 lines).


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D151761/new/

https://reviews.llvm.org/D151761

Files:
  clang/docs/ClangFormatStyleOptions.rst
  clang/docs/ReleaseNotes.rst
  clang/include/clang/Format/Format.h
  clang/lib/Format/Format.cpp
  clang/lib/Format/WhitespaceManager.cpp
  clang/lib/Format/WhitespaceManager.h
  clang/unittests/Format/ConfigParseTest.cpp
  clang/unittests/Format/FormatTest.cpp

Index: clang/unittests/Format/FormatTest.cpp
===
--- clang/unittests/Format/FormatTest.cpp
+++ clang/unittests/Format/FormatTest.cpp
@@ -19244,6 +19244,255 @@
BracedAlign);
 }
 
+TEST_F(FormatTest, AlignConsecutiveShortCaseStatements) {
+  FormatStyle Alignment = getLLVMStyle();
+  Alignment.AllowShortCaseLabelsOnASingleLine = true;
+  Alignment.AlignConsecutiveShortCaseStatements.Enabled = true;
+
+  verifyFormat("switch (level) {\n"
+   "case log::info:return \"info\";\n"
+   "case log::warning: return \"warning\";\n"
+   "default:   return \"default\";\n"
+   "}",
+   Alignment);
+
+  verifyFormat("switch (level) {\n"
+   "case log::info:return \"info\";\n"
+   "case log::warning: return \"warning\";\n"
+   "}",
+   "switch (level) {\n"
+   "case log::info: return \"info\";\n"
+   "case log::warning:\n"
+   "  return \"warning\";\n"
+   "}",
+   Alignment);
+
+  // Empty case statements push out the alignment, but non-short case labels don't.
+  verifyFormat("switch (level) {\n"
+   "case log::info: return \"info\";\n"
+   "case log::critical:\n"
+   "case log::warning:\n"
+   "case log::severe:   return \"severe\";\n"
+   "case log::extra_severe:\n" 
+   "  // comment\n"
+   "  return \"extra_severe\";\n"
+   "}",
+   Alignment);
+
+  // Verify comments and empty lines break the alignment.
+  verifyFormat("switch (level) {\n"
+   "case log::info:return \"info\";\n"
+   "case log::warning: return \"warning\";\n"
+   "// comment\n"
+   "case log::critical: return \"critical\";\n"
+   "default:return \"default\";\n"
+   "\n"
+   "case log::severe: return \"severe\";\n"
+   "}",
+   "switch (level) {\n"
+   "case log::info:return \"info\";\n"
+   "case log::warning: return \"warning\";\n"
+   "// comment\n"
+   "case log::critical: return \"critical\";\n"
+   "default:return \"default\";\n"
+   "\n"
+   "case log::severe: return \"severe\";\n"
+   "}",
+   Alignment);
+
+  // Empty case statements don't break the alignment, and potentially push it out
+  verifyFormat("switch (level) {\n"
+   "case log::info: return \"info\";\n"
+   "case log::warning:\n"
+   "case log::critical:\n"
+   "default:return \"default\";\n"
+   "}",
+   Alignment);
+
+  // Implicit fallthrough cases can be aligned with either a comment or
+  // [[fallthrough]]
+  verifyFormat("switch (level) {\n"
+   "case log::info: return \"info\";\n"
+   "case log::warning:  // fallthrough\n"
+   "case log::error:return \"error\";\n"
+   "case log::critical: /*fallthrough*/\n"
+   "case log::severe:   return \"severe\";\n"
+   "case log::diag: [[fallthrough]];\n"
+   "default:return \"default\";\n"
+   "}",
+   Alignment);
+
+  // Verify trailing comment that needs a reflow also gets aligned properly.
+  

[PATCH] D151761: clang-format: Add AlignConsecutiveShortCaseStatements

[Owen Pan via Phabricator via cfe-commits]

owenpan added a comment.

In D151761#4418809 , 
@HazardyKnusperkeks wrote:

> I think we need some input what should be able to be aligned - the statement 
> vs. the colon - and only //short// case statements vs all case statements.

I think we should align the colons only for short case statements and don't 
align if `AllowShortCaseLabelsOnASingleLine` is false.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D151761/new/

https://reviews.llvm.org/D151761

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

[PATCH] D151761: clang-format: Add AlignConsecutiveShortCaseStatements

[Björn Schäpers via Phabricator via cfe-commits]

HazardyKnusperkeks added a comment.

In D151761#4416224 , @galenelias 
wrote:

> Well, I guess I didn't put quite enough thought into the `AlignCaseColons` 
> option.  While this solves the empty case label problem, it will also match 
> in non-short case label scenarios such as the following, which doesn't seem 
> desirable:
>
>   switch (level) {
>   case log::info  :
>   case log::error :
> return true;
>   case log::none :
>   case log::warning  :
>   case log::critical :
> return false;
>   }
>
> In order to solve this (and the other) issues, I think the only solution is 
> to roll a custom alignment method instead of using `AlignTokens`, so that the 
> alignment can be more correctly based on the detection of short case 
> statements.
>
> This is going to take considerably more time and code, so not sure when I'll 
> be able to work on it.

For me this would actually be desired. But from the name `ShortCaseStatements` 
I can see that it may be not what we want. This could be //fixed// with just 
renaming to `ConsecutiveCaseStatements`. :)

In D151761#4415755 , @galenelias 
wrote:

> Ok, I added the ability to align the case label colons.  In your original 
> message you mentioned "I'd like to align the colon (and thus the statement 
> behind that)" which implies actually adding the whitespace after the 'case' 
> token itself.  Not sure if that would still be your preference in an ideal 
> world, or if I just misinterpreted your request.  Aligning the colons 
> themselves is very straightforward.
>
> I opted to make this an option on `AlignConsecutiveStyle`, as that is 
> consistent with how we customize some of the other AlignConsecutive* options, 
> and it seemed awkward to add a floating top level boolean config option which 
> applied to just this scenario - although it has the similar downside that it 
> muddies the AlignConsecutiveStyle options for the other use cases.

I have no problem with that, but the full disclosure is: @MyDeveloperDay  isn't 
happy with e.g. `PadOperators` and thus will most likely not like this.

I think we need some input what should be able to be aligned - the statement 
vs. the colon - and only //short// case statements vs all case statements.




Comment at: clang/include/clang/Format/Format.h:257
+/// \endcode
+bool AlignCaseColons;
 bool operator==(const AlignConsecutiveStyle ) const {

I'd also sort this alphabetically.



Comment at: clang/lib/Format/WhitespaceManager.cpp:881
+  return C.Tok->is(TT_CaseLabelColon);
+} else {
+  // Ignore 'InsideToken' to allow matching trailing comments which

else after return can be dropped.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D151761/new/

https://reviews.llvm.org/D151761

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

[PATCH] D151761: clang-format: Add AlignConsecutiveShortCaseStatements

[Owen Pan via Phabricator via cfe-commits]

owenpan added a comment.

In D151761#4410998 , 
@HazardyKnusperkeks wrote:

> In D151761#4410158 , @galenelias 
> wrote:
>
>> Yah, I think leaving it open would be my preference at this point.  Not sure 
>> how to properly document that though?  Just be explicit in the tests?  
>> Mention it in `alignConsecutiveShortCaseStatements`?  Should it be 
>> documented in the generated documentation (that feels a bit heavy)?
>
> It will be a known issue, and thus it should be documented explicitly in my 
> opinion. Otherwise there will be bug reports. So I would do:
>
> - Mention it in the options documentation.
> - Add a FIXME comment where the fix most likely would be applied.
> - Comment on test cases that they have to be changed once this is fixed - or 
> add tests which are within an `#if 0`, don't know what @MyDeveloperDay or 
> @owenpan like better.

I prefer the latter, and IIRC @MyDeveloperDay doesn't use the former.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D151761/new/

https://reviews.llvm.org/D151761

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

[PATCH] D151761: clang-format: Add AlignConsecutiveShortCaseStatements

[Galen Elias via Phabricator via cfe-commits]

galenelias added a comment.

Well, I guess I didn't put quite enough thought into the `AlignCaseColons` 
option.  While this solves the empty case label problem, it will also match in 
non-short case label scenarios such as the following, which doesn't seem 
desirable:

  switch (level) {
  case log::info  :
  case log::error :
return true;
  case log::none :
  case log::warning  :
  case log::critical :
return false;
  }

In order to solve this (and the other) issues, I think the only solution is to 
roll a custom alignment method instead of using `AlignTokens`, so that the 
alignment can be more correctly based on the detection of short case statements.

This is going to take considerably more time and code, so not sure when I'll be 
able to work on it.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D151761/new/

https://reviews.llvm.org/D151761

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

[PATCH] D151761: clang-format: Add AlignConsecutiveShortCaseStatements

[Galen Elias via Phabricator via cfe-commits]

galenelias updated this revision to Diff 530707.
galenelias edited the summary of this revision.
galenelias added a comment.

Ok, I added the ability to align the case label colons.  In your original 
message you mentioned "I'd like to align the colon (and thus the statement 
behind that)" which implies actually adding the whitespace after the 'case' 
token itself.  Not sure if that would still be your preference in an ideal 
world, or if I just misinterpreted your request.  Aligning the colons 
themselves is very straightforward.

I opted to make this an option on `AlignConsecutiveStyle`, as that is 
consistent with how we customize some of the other AlignConsecutive* options, 
and it seemed awkward to add a floating top level boolean config option which 
applied to just this scenario - although it has the similar downside that it 
muddies the AlignConsecutiveStyle options for the other use cases.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D151761/new/

https://reviews.llvm.org/D151761

Files:
  clang/docs/ClangFormatStyleOptions.rst
  clang/docs/ReleaseNotes.rst
  clang/include/clang/Format/Format.h
  clang/lib/Format/Format.cpp
  clang/lib/Format/WhitespaceManager.cpp
  clang/lib/Format/WhitespaceManager.h
  clang/unittests/Format/ConfigParseTest.cpp
  clang/unittests/Format/FormatTest.cpp

Index: clang/unittests/Format/FormatTest.cpp
===
--- clang/unittests/Format/FormatTest.cpp
+++ clang/unittests/Format/FormatTest.cpp
@@ -19192,6 +19192,228 @@
BracedAlign);
 }
 
+TEST_F(FormatTest, AlignConsecutiveShortCaseStatements) {
+  FormatStyle Alignment = getLLVMStyle();
+  Alignment.AllowShortCaseLabelsOnASingleLine = true;
+  Alignment.AlignConsecutiveShortCaseStatements.Enabled = true;
+  verifyFormat("switch (level) {\n"
+   "case log::info:return \"info\";\n"
+   "case log::warning: return \"warning\";\n"
+   "default:   return \"default\";\n"
+   "}",
+   Alignment);
+
+  verifyFormat("switch (level) {\n"
+   "case log::info:return \"info\";\n"
+   "case log::warning: return \"warning\";\n"
+   "}",
+   "switch (level) {\n"
+   "case log::info: return \"info\";\n"
+   "case log::warning:\n"
+   "  return \"warning\";\n"
+   "}",
+   Alignment);
+
+  // Verify comments and empty lines break the alignment.
+  verifyFormat("switch (level) {\n"
+   "case log::info:return \"info\";\n"
+   "case log::warning: return \"warning\";\n"
+   "// comment\n"
+   "case log::critical: return \"critical\";\n"
+   "default:return \"default\";\n"
+   "\n"
+   "case log::severe: return \"severe\";\n"
+   "}",
+   "switch (level) {\n"
+   "case log::info:return \"info\";\n"
+   "case log::warning: return \"warning\";\n"
+   "// comment\n"
+   "case log::critical: return \"critical\";\n"
+   "default:return \"default\";\n"
+   "\n"
+   "case log::severe: return \"severe\";\n"
+   "}",
+   Alignment);
+
+  // Empty case statements (implicit fallthrough) currently break the alignment.
+  verifyFormat("switch (level) {\n"
+   "case log::info: return \"info\";\n"
+   "case log::warning:\n"
+   "default: return \"default\";\n"
+   "}",
+   Alignment);
+
+  // Implicit fallthrough cases can be aligned with either a comment or
+  // [[fallthrough]]
+  verifyFormat("switch (level) {\n"
+   "case log::info: return \"info\";\n"
+   "case log::warning:  // fallthrough\n"
+   "case log::error:return \"error\";\n"
+   "case log::critical: /*fallthrough*/\n"
+   "case log::severe:   return \"severe\";\n"
+   "case log::diag: [[fallthrough]];\n"
+   "default:return \"default\";\n"
+   "}",
+   Alignment);
+
+  // Verify trailing comment that needs a reflow also gets aligned properly.
+  verifyFormat("switch (level) {\n"
+   "case log::info:return \"info\";\n"
+   "case log::warning: // fallthrough\n"
+   "case log::error:   return \"error\";\n"
+   "}",
+   "switch (level) {\n"
+   "case log::info:return \"info\";\n"
+   "case log::warning: //fallthrough\n"
+   "case log::error:   return \"error\";\n"
+   "}",
+   Alignment);
+
+  // Verify adjacent non-short case statements don't change the alignment, and
+  // properly break the set of consecutive statements.
+  verifyFormat("switch (level) {\n"
+ 

[PATCH] D151761: clang-format: Add AlignConsecutiveShortCaseStatements

[Björn Schäpers via Phabricator via cfe-commits]

HazardyKnusperkeks added a comment.

After I came across some of my code, where I'd really like a feature like this, 
I'm back at wishing to align the colons. Same as in a bit field. And when you 
would do that I think there would be no open issue with empty statements, right?

So for my code I'd favor

  switch (level) {
  case log::info: return "info: ";
  case log::warning : return "warning:  ";
  case log::error   : return "error:";
  case log::critical: return "critical: ";
  default   : return "";
  }

over

  switch (level) {
  case log::info: return "info: ";
  case log::warning:  return "warning:  ";
  case log::error:return "error:";
  case log::critical: return "critical: ";
  default:return "";
  }

(For me actually there would always be at least one space before the colon.)
Maybe think about it?


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D151761/new/

https://reviews.llvm.org/D151761

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

[PATCH] D151761: clang-format: Add AlignConsecutiveShortCaseStatements

[Björn Schäpers via Phabricator via cfe-commits]

HazardyKnusperkeks added a comment.

In D151761#4410158 , @galenelias 
wrote:

> Yah, I think leaving it open would be my preference at this point.  Not sure 
> how to properly document that though?  Just be explicit in the tests?  
> Mention it in `alignConsecutiveShortCaseStatements`?  Should it be documented 
> in the generated documentation (that feels a bit heavy)?

It will be a known issue, and thus it should be documented explicitly in my 
opinion. Otherwise there will be bug reports. So I would do:

- Mention it in the options documentation.
- Add a FIXME comment where the fix most likely would be applied.
- Comment on test cases that they have to be changed once this is fixed - or 
add tests which are within an `#if 0`, don't know what @MyDeveloperDay or 
@owenpan like better.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D151761/new/

https://reviews.llvm.org/D151761

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

[PATCH] D151761: clang-format: Add AlignConsecutiveShortCaseStatements

[Galen Elias via Phabricator via cfe-commits]

galenelias added a comment.

In D151761#4404179 , 
@HazardyKnusperkeks wrote:

> In D151761#4403828 , @galenelias 
> wrote:
>
>> In D151761#4400693 , 
>> @HazardyKnusperkeks wrote:
>>
>>> I'd say: align it.
>>
>> If it was straightforward, I would definitely agree to just align across 
>> empty case labels, but unfortunately there is no way to do that while using 
>> the generic AlignTokens (since the line with just a case has no token which 
>> matches the pattern, but needs to not break the alignment streak).  I guess 
>> the way to do it generically would be to add some sort of ExcludeLine lambda 
>> to allow filtering out lines.  Given that I don't think this is a common 
>> pattern with these 'Short Case Labels', and it's fairly easy to work around 
>> with `[[fallthrough]];` my plan is just to leave this as is (and add a test 
>> to make the behavior explicit).
>
> Leaving it open (and documenting that) I could get behind, but making it 
> explicit as desired behavior will not get my approval. On the other hand I 
> will not stand in the way, if the others approve.

Yah, I think leaving it open would be my preference at this point.  Not sure 
how to properly document that though?  Just be explicit in the tests?  Mention 
it in `alignConsecutiveShortCaseStatements`?  Should it be documented in the 
generated documentation (that feels a bit heavy)?


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D151761/new/

https://reviews.llvm.org/D151761

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

[PATCH] D151761: clang-format: Add AlignConsecutiveShortCaseStatements

[Björn Schäpers via Phabricator via cfe-commits]

HazardyKnusperkeks added a comment.

In D151761#4403828 , @galenelias 
wrote:

> In D151761#4400693 , 
> @HazardyKnusperkeks wrote:
>
>> I'd say: align it.
>
> If it was straightforward, I would definitely agree to just align across 
> empty case labels, but unfortunately there is no way to do that while using 
> the generic AlignTokens (since the line with just a case has no token which 
> matches the pattern, but needs to not break the alignment streak).  I guess 
> the way to do it generically would be to add some sort of ExcludeLine lambda 
> to allow filtering out lines.  Given that I don't think this is a common 
> pattern with these 'Short Case Labels', and it's fairly easy to work around 
> with `[[fallthrough]];` my plan is just to leave this as is (and add a test 
> to make the behavior explicit).

Leaving it open (and documenting that) I could get behind, but making it 
explicit as desired behavior will not get my approval. On the other hand I will 
not stand in the way, if the others approve.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D151761/new/

https://reviews.llvm.org/D151761

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

[PATCH] D151761: clang-format: Add AlignConsecutiveShortCaseStatements

[Galen Elias via Phabricator via cfe-commits]

galenelias updated this revision to Diff 529365.

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D151761/new/

https://reviews.llvm.org/D151761

Files:
  clang/docs/ClangFormatStyleOptions.rst
  clang/docs/ReleaseNotes.rst
  clang/include/clang/Format/Format.h
  clang/lib/Format/Format.cpp
  clang/lib/Format/WhitespaceManager.cpp
  clang/lib/Format/WhitespaceManager.h
  clang/unittests/Format/ConfigParseTest.cpp
  clang/unittests/Format/FormatTest.cpp

Index: clang/unittests/Format/FormatTest.cpp
===
--- clang/unittests/Format/FormatTest.cpp
+++ clang/unittests/Format/FormatTest.cpp
@@ -19192,6 +19192,181 @@
BracedAlign);
 }
 
+TEST_F(FormatTest, AlignConsecutiveShortCaseStatements) {
+  FormatStyle Alignment = getLLVMStyle();
+  Alignment.AllowShortCaseLabelsOnASingleLine = true;
+  Alignment.AlignConsecutiveShortCaseStatements.Enabled = true;
+  verifyFormat("switch (level) {\n"
+   "case log::info:return \"info\";\n"
+   "case log::warning: return \"warning\";\n"
+   "default:   return \"default\";\n"
+   "}",
+   Alignment);
+
+  verifyFormat("switch (level) {\n"
+   "case log::info:return \"info\";\n"
+   "case log::warning: return \"warning\";\n"
+   "}",
+   "switch (level) {\n"
+   "case log::info: return \"info\";\n"
+   "case log::warning:\n"
+   "  return \"warning\";\n"
+   "}",
+   Alignment);
+
+  // Verify comments and empty lines break the alignment.
+  verifyFormat("switch (level) {\n"
+   "case log::info:return \"info\";\n"
+   "case log::warning: return \"warning\";\n"
+   "// comment\n"
+   "case log::critical: return \"critical\";\n"
+   "default:return \"default\";\n"
+   "\n"
+   "case log::severe: return \"severe\";\n"
+   "}",
+   "switch (level) {\n"
+   "case log::info:return \"info\";\n"
+   "case log::warning: return \"warning\";\n"
+   "// comment\n"
+   "case log::critical: return \"critical\";\n"
+   "default:return \"default\";\n"
+   "\n"
+   "case log::severe: return \"severe\";\n"
+   "}",
+   Alignment);
+
+  // Empty case statements (implicit fallthrough) currently break the alignment.
+  verifyFormat("switch (level) {\n"
+   "case log::info: return \"info\";\n"
+   "case log::warning:\n"
+   "default: return \"default\";\n"
+   "}",
+   Alignment);
+
+  // Implicit fallthrough cases can be aligned with either a comment or
+  // [[fallthrough]]
+  verifyFormat("switch (level) {\n"
+   "case log::info: return \"info\";\n"
+   "case log::warning:  // fallthrough\n"
+   "case log::error:return \"error\";\n"
+   "case log::critical: /*fallthrough*/\n"
+   "case log::severe:   return \"severe\";\n"
+   "case log::diag: [[fallthrough]];\n"
+   "default:return \"default\";\n"
+   "}",
+   Alignment);
+
+  // Verify trailing comment that needs a reflow also gets aligned properly.
+  verifyFormat("switch (level) {\n"
+   "case log::info:return \"info\";\n"
+   "case log::warning: // fallthrough\n"
+   "case log::error:   return \"error\";\n"
+   "}",
+   "switch (level) {\n"
+   "case log::info:return \"info\";\n"
+   "case log::warning: //fallthrough\n"
+   "case log::error:   return \"error\";\n"
+   "}",
+   Alignment);
+
+  // Verify adjacent non-short case statements don't change the alignment, and
+  // properly break the set of consecutive statements.
+  verifyFormat("switch (level) {\n"
+   "case log::critical:\n"
+   "  // comment\n"
+   "  return \"critical\";\n"
+   "case log::info:return \"info\";\n"
+   "case log::warning: return \"warning\";\n"
+   "default:\n"
+   "  // comment\n"
+   "  return \"\";\n"
+   "case log::error:  return \"error\";\n"
+   "case log::severe: return \"severe\";\n"
+   "case log::extra_critical:\n"
+   "  // comment\n"
+   "  return \"extra critical\";\n"
+   "}",
+   Alignment);
+
+  Alignment.SpaceBeforeCaseColon = true;
+  verifyFormat("switch (level) {\n"
+   "case log::info :return \"info\";\n"
+   "case log::warning : return \"warning\";\n"
+   "default :   return \"default\";\n"
+ 

[PATCH] D151761: clang-format: Add AlignConsecutiveShortCaseStatements

[Galen Elias via Phabricator via cfe-commits]

galenelias added a comment.

In D151761#4400693 , 
@HazardyKnusperkeks wrote:

> In D151761#4400056 , @galenelias 
> wrote:
>
>> In D151761#4394758 , 
>> @MyDeveloperDay wrote:
>>
>>> did you consider a case where the case falls through? (i.e. no return)
>>>
>>>   "case log::info :return \"info\";\n"
>>>   "case log::warning :\n"
>>>   "default :   return \"default\";\n"
>>
>> That's a great point.  I didn't really consider this, and currently this 
>> particular case won't align the case statements if they have an empty case 
>> block, however if you had some tokens (e.g. `// fallthrough`) it would.  
>> It's not immediately clear to me what the expectation would be.  I guess to 
>> align as if there was an invisible trailing token, but it's a bit awkward if 
>> the cases missing a body are the 'long' cases that push out the alignment.  
>> Also, I don't think it's possible to use `AlignTokens` and get this 
>> behavior, as there is no token on those lines to align, so it's not 
>> straightforward to handle.  I guess I'll be curious to see if there is 
>> feedback or cases where this behavior is desired, and if so, I can look into 
>> adding that functionality later.  Since right now it would involve a 
>> completely custom AlignTokens clone, my preference would be to just leave 
>> this as not supported.
>
> I'd say: align it.

If it was straightforward, I would definitely agree to just align across empty 
case labels, but unfortunately there is no way to do that while using the 
generic AlignTokens (since the line with just a case has no token which matches 
the pattern, but needs to not break the alignment streak).  I guess the way to 
do it generically would be to add some sort of ExcludeLine lambda to allow 
filtering out lines.  Given that I don't think this is a common pattern with 
these 'Short Case Labels', and it's fairly easy to work around with 
`[[fallthrough]];` my plan is just to leave this as is (and add a test to make 
the behavior explicit).

In D151761#4402568 , @MyDeveloperDay 
wrote:

> In D151761#4400056 , @galenelias 
> wrote:
>
>> In D151761#4394758 , 
>> @MyDeveloperDay wrote:
>>
>>> did you consider a case where the case falls through? (i.e. no return)
>>>
>>>   "case log::info :return \"info\";\n"
>>>   "case log::warning :\n"
>>>   "default :   return \"default\";\n"
>>
>> That's a great point.  I didn't really consider this, and currently this 
>> particular case won't align the case statements if they have an empty case 
>> block, however if you had some tokens (e.g. `// fallthrough`) it would.  
>> It's not immediately clear to me what the expectation would be.  I guess to 
>> align as if there was an invisible trailing token, but it's a bit awkward if 
>> the cases missing a body are the 'long' cases that push out the alignment.  
>> Also, I don't think it's possible to use `AlignTokens` and get this 
>> behavior, as there is no token on those lines to align, so it's not 
>> straightforward to handle.  I guess I'll be curious to see if there is 
>> feedback or cases where this behavior is desired, and if so, I can look into 
>> adding that functionality later.  Since right now it would involve a 
>> completely custom AlignTokens clone, my preference would be to just leave 
>> this as not supported.
>
> Can you add a unit test with a // fallthough comment, and a /* FALLTHROUGH */ 
> and [[fallthrough]] so we know whats going to happen in those cases at a 
> minimum.

Done.  This actually caught an issue I wasn't aware of where if you use 
`//fallthrough` and the comment needs to be reflowed, you get two `Change` 
entries pointing to the same comment token, which throws off the algorithm, and 
results in broken alignment.  Attempted to fix it up to ignore the inner reflow 
change and it seems to work in my basic validation.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D151761/new/

https://reviews.llvm.org/D151761

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

[PATCH] D151761: clang-format: Add AlignConsecutiveShortCaseStatements

[MyDeveloperDay via Phabricator via cfe-commits]

MyDeveloperDay added a comment.

In D151761#4400056 , @galenelias 
wrote:

> In D151761#4394758 , 
> @MyDeveloperDay wrote:
>
>> did you consider a case where the case falls through? (i.e. no return)
>>
>>   "case log::info :return \"info\";\n"
>>   "case log::warning :\n"
>>   "default :   return \"default\";\n"
>
> That's a great point.  I didn't really consider this, and currently this 
> particular case won't align the case statements if they have an empty case 
> block, however if you had some tokens (e.g. `// fallthrough`) it would.  It's 
> not immediately clear to me what the expectation would be.  I guess to align 
> as if there was an invisible trailing token, but it's a bit awkward if the 
> cases missing a body are the 'long' cases that push out the alignment.  Also, 
> I don't think it's possible to use `AlignTokens` and get this behavior, as 
> there is no token on those lines to align, so it's not straightforward to 
> handle.  I guess I'll be curious to see if there is feedback or cases where 
> this behavior is desired, and if so, I can look into adding that 
> functionality later.  Since right now it would involve a completely custom 
> AlignTokens clone, my preference would be to just leave this as not supported.

Can you add a unit test with a // fallthough comment, and a /* FALLTHROUGH */ 
and [[fallthrough]] so we know whats going to happen in those cases at a 
minimum.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D151761/new/

https://reviews.llvm.org/D151761

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

[PATCH] D151761: clang-format: Add AlignConsecutiveShortCaseStatements

[Björn Schäpers via Phabricator via cfe-commits]

HazardyKnusperkeks requested changes to this revision.
HazardyKnusperkeks added a comment.
This revision now requires changes to proceed.

In D151761#4400056 , @galenelias 
wrote:

> In D151761#4394758 , 
> @MyDeveloperDay wrote:
>
>> did you consider a case where the case falls through? (i.e. no return)
>>
>>   "case log::info :return \"info\";\n"
>>   "case log::warning :\n"
>>   "default :   return \"default\";\n"
>
> That's a great point.  I didn't really consider this, and currently this 
> particular case won't align the case statements if they have an empty case 
> block, however if you had some tokens (e.g. `// fallthrough`) it would.  It's 
> not immediately clear to me what the expectation would be.  I guess to align 
> as if there was an invisible trailing token, but it's a bit awkward if the 
> cases missing a body are the 'long' cases that push out the alignment.  Also, 
> I don't think it's possible to use `AlignTokens` and get this behavior, as 
> there is no token on those lines to align, so it's not straightforward to 
> handle.  I guess I'll be curious to see if there is feedback or cases where 
> this behavior is desired, and if so, I can look into adding that 
> functionality later.  Since right now it would involve a completely custom 
> AlignTokens clone, my preference would be to just leave this as not supported.

I'd say: align it.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D151761/new/

https://reviews.llvm.org/D151761

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

[PATCH] D151761: clang-format: Add AlignConsecutiveShortCaseStatements

[Galen Elias via Phabricator via cfe-commits]

galenelias marked an inline comment as done.
galenelias added a comment.

In D151761#4394758 , @MyDeveloperDay 
wrote:

> did you consider a case where the case falls through? (i.e. no return)
>
>   "case log::info :return \"info\";\n"
>   "case log::warning :\n"
>   "default :   return \"default\";\n"

That's a great point.  I didn't really consider this, and currently this 
particular case won't align the case statements if they have an empty case 
block, however if you had some tokens (e.g. `// fallthrough`) it would.  It's 
not immediately clear to me what the expectation would be.  I guess to align as 
if there was an invisible trailing token, but it's a bit awkward if the cases 
missing a body are the 'long' cases that push out the alignment.  Also, I don't 
think it's possible to use `AlignTokens` and get this behavior, as there is no 
token on those lines to align, so it's not straightforward to handle.  I guess 
I'll be curious to see if there is feedback or cases where this behavior is 
desired, and if so, I can look into adding that functionality later.  Since 
right now it would involve a completely custom AlignTokens clone, my preference 
would be to just leave this as not supported.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D151761/new/

https://reviews.llvm.org/D151761

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

[PATCH] D151761: clang-format: Add AlignConsecutiveShortCaseStatements

[Galen Elias via Phabricator via cfe-commits]

galenelias marked 3 inline comments as done.
galenelias added inline comments.



Comment at: clang/docs/ClangFormatStyleOptions.rst:798
+
+case log::info: return "info:";
+case log::warning:  return "warning:";

MyDeveloperDay wrote:
> Do you think the documentation should give examples of the the other cases?
Hmm, I'm not sure what other cases?  Cases where it doesn't align?  In general, 
for the 'Align*' options we only show examples of the resulting formatting.



Comment at: clang/docs/ClangFormatStyleOptions.rst:790
+
+**AlignConsecutiveShortCaseLabels** (``AlignConsecutiveStyle``) 
:versionbadge:`clang-format 17` :ref:`¶ `
+  Style of aligning consecutive short case labels.

MyDeveloperDay wrote:
> HazardyKnusperkeks wrote:
> > MyDeveloperDay wrote:
> > > galenelias wrote:
> > > > MyDeveloperDay wrote:
> > > > > Did you generate this by hand or run the dump_format_style.py 
> > > > > function? Format.h and the rst look out of sync
> > > > This was generated from dump_format_style.py.  What looks out of sync?  
> > > > My guess is the confusing thing is all the styles which use 
> > > > `AlignConsecutiveStyle` get the same documentation pasted for them 
> > > > which specifically references `AlignConsecutiveMacros` and makes it 
> > > > look like it's for the wrong option.
> > > that doesn't feel right to me.. not saying its not dump_format_style.py 
> > > but it shouldn't do that in my view
> > What shouldn't it be doing?
> > The struct is used for multiple options and the documentation is that ways 
> > since we have the struct.
> I understand why its there, I just don't like that the text is repeated and 
> doesn't really reference the case statements, (it could be confusing).
> 
> for example what would `PadOperators` mean in this case
> 
> 
I agree that it's confusing, I've actually gotten confused a few times while 
reading the documentation for various 'AlignConsecutive*' options.  
PadOperators and AlignCompound both explicitly state they only apply to 
`AlignConsecutiveAssignments`.  Not sure what the fix would be, as this seems 
to be a fairly by design aspect of the struct sharing with regards to the 
documentation generation.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D151761/new/

https://reviews.llvm.org/D151761

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

[PATCH] D151761: clang-format: Add AlignConsecutiveShortCaseStatements

[MyDeveloperDay via Phabricator via cfe-commits]

MyDeveloperDay added a comment.

did you consider a case where the case falls through? (i.e. no return)

  "case log::info :return \"info\";\n"
  "case log::warning :\n"
  "default :   return \"default\";\n"


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D151761/new/

https://reviews.llvm.org/D151761

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

[PATCH] D151761: clang-format: Add AlignConsecutiveShortCaseStatements

[MyDeveloperDay via Phabricator via cfe-commits]

MyDeveloperDay added inline comments.



Comment at: clang/docs/ClangFormatStyleOptions.rst:798
+
+case log::info: return "info:";
+case log::warning:  return "warning:";

Do you think the documentation should give examples of the the other cases?


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D151761/new/

https://reviews.llvm.org/D151761

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

[PATCH] D151761: clang-format: Add AlignConsecutiveShortCaseStatements

[MyDeveloperDay via Phabricator via cfe-commits]

MyDeveloperDay added a comment.

Apart from the documentation this looks fine.. thats probably outside the scope 
of this change


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D151761/new/

https://reviews.llvm.org/D151761

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

[PATCH] D151761: clang-format: Add AlignConsecutiveShortCaseStatements

[MyDeveloperDay via Phabricator via cfe-commits]

MyDeveloperDay added inline comments.



Comment at: clang/docs/ClangFormatStyleOptions.rst:790
+
+**AlignConsecutiveShortCaseLabels** (``AlignConsecutiveStyle``) 
:versionbadge:`clang-format 17` :ref:`¶ `
+  Style of aligning consecutive short case labels.

HazardyKnusperkeks wrote:
> MyDeveloperDay wrote:
> > galenelias wrote:
> > > MyDeveloperDay wrote:
> > > > Did you generate this by hand or run the dump_format_style.py function? 
> > > > Format.h and the rst look out of sync
> > > This was generated from dump_format_style.py.  What looks out of sync?  
> > > My guess is the confusing thing is all the styles which use 
> > > `AlignConsecutiveStyle` get the same documentation pasted for them which 
> > > specifically references `AlignConsecutiveMacros` and makes it look like 
> > > it's for the wrong option.
> > that doesn't feel right to me.. not saying its not dump_format_style.py but 
> > it shouldn't do that in my view
> What shouldn't it be doing?
> The struct is used for multiple options and the documentation is that ways 
> since we have the struct.
I understand why its there, I just don't like that the text is repeated and 
doesn't really reference the case statements, (it could be confusing).

for example what would `PadOperators` mean in this case




CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D151761/new/

https://reviews.llvm.org/D151761

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

[PATCH] D151761: clang-format: Add AlignConsecutiveShortCaseStatements

[Björn Schäpers via Phabricator via cfe-commits]

HazardyKnusperkeks accepted this revision.
HazardyKnusperkeks added a comment.
This revision is now accepted and ready to land.

Please wait for some other approval (or comments) for a few days.




Comment at: clang/unittests/Format/FormatTest.cpp:19218
+  // Verify comments and empty lines break the alignment
+  verifyFormat("switch (level) {\n"
+   "case log::info:return \"info\";\n"

galenelias wrote:
> HazardyKnusperkeks wrote:
> > If both strings are the same, you only need to supply it once.
> > 
> > Or are they different and I can't see it?
> They are the same, but I can't use the single string version, as that one 
> calls test::messUp on the string which will strip blank lines, which then 
> invalidates the test case.  It seems pretty much all tests which are trying 
> to validate behavior around empty spaces has to use the two string version.
Check.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D151761/new/

https://reviews.llvm.org/D151761

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

[PATCH] D151761: clang-format: Add AlignConsecutiveShortCaseStatements

[Galen Elias via Phabricator via cfe-commits]

galenelias marked 3 inline comments as done.
galenelias added inline comments.



Comment at: clang/unittests/Format/FormatTest.cpp:19218
+  // Verify comments and empty lines break the alignment
+  verifyFormat("switch (level) {\n"
+   "case log::info:return \"info\";\n"

HazardyKnusperkeks wrote:
> If both strings are the same, you only need to supply it once.
> 
> Or are they different and I can't see it?
They are the same, but I can't use the single string version, as that one calls 
test::messUp on the string which will strip blank lines, which then invalidates 
the test case.  It seems pretty much all tests which are trying to validate 
behavior around empty spaces has to use the two string version.



Comment at: clang/unittests/Format/FormatTest.cpp:19257
+
+  Alignment.ColumnLimit = 80;
+  Alignment.SpaceBeforeCaseColon = true;

HazardyKnusperkeks wrote:
> For what is this?
Oops, I was trying to set ColumnLimit = 0 before to stop it from merging 
multi-line case statements, but it wasn't working as expected, so I ended up 
inserting comments instead, and forgot to remove resetting this.  Good catch!



Comment at: clang/unittests/Format/FormatTest.cpp:19206
+
+  // Make sure we don't incorrectly align correctly across nested switch cases
+  EXPECT_EQ("switch (level) {\n"

HazardyKnusperkeks wrote:
> 
I'll get used to adding full stops eventually... sorry for not catching this.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D151761/new/

https://reviews.llvm.org/D151761

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

[PATCH] D151761: clang-format: Add AlignConsecutiveShortCaseStatements

[Galen Elias via Phabricator via cfe-commits]

galenelias updated this revision to Diff 528118.
galenelias added a comment.

Fixup up review comments.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D151761/new/

https://reviews.llvm.org/D151761

Files:
  clang/docs/ClangFormatStyleOptions.rst
  clang/docs/ReleaseNotes.rst
  clang/include/clang/Format/Format.h
  clang/lib/Format/Format.cpp
  clang/lib/Format/WhitespaceManager.cpp
  clang/lib/Format/WhitespaceManager.h
  clang/unittests/Format/ConfigParseTest.cpp
  clang/unittests/Format/FormatTest.cpp

Index: clang/unittests/Format/FormatTest.cpp
===
--- clang/unittests/Format/FormatTest.cpp
+++ clang/unittests/Format/FormatTest.cpp
@@ -19192,6 +19192,147 @@
BracedAlign);
 }
 
+TEST_F(FormatTest, AlignConsecutiveShortCaseStatements) {
+  FormatStyle Alignment = getLLVMStyle();
+  Alignment.AllowShortCaseLabelsOnASingleLine = true;
+  Alignment.AlignConsecutiveShortCaseStatements.Enabled = true;
+  verifyFormat("switch (level) {\n"
+   "case log::info:return \"info\";\n"
+   "case log::warning: return \"warning\";\n"
+   "default:   return \"default\";\n"
+   "}",
+   Alignment);
+
+  verifyFormat("switch (level) {\n"
+   "case log::info:return \"info\";\n"
+   "case log::warning: return \"warning\";\n"
+   "}",
+   "switch (level) {\n"
+   "case log::info: return \"info\";\n"
+   "case log::warning:\n"
+   "  return \"warning\";\n"
+   "}",
+   Alignment);
+
+  // Verify comments and empty lines break the alignment.
+  verifyFormat("switch (level) {\n"
+   "case log::info:return \"info\";\n"
+   "case log::warning: return \"warning\";\n"
+   "// comment\n"
+   "case log::critical: return \"critical\";\n"
+   "default:return \"default\";\n"
+   "\n"
+   "case log::severe: return \"severe\";\n"
+   "}",
+   "switch (level) {\n"
+   "case log::info:return \"info\";\n"
+   "case log::warning: return \"warning\";\n"
+   "// comment\n"
+   "case log::critical: return \"critical\";\n"
+   "default:return \"default\";\n"
+   "\n"
+   "case log::severe: return \"severe\";\n"
+   "}",
+   Alignment);
+
+  // Verify adjacent non-short case statements don't change the alignment, and
+  // properly break the set of consecutive statements.
+  verifyFormat("switch (level) {\n"
+   "case log::critical:\n"
+   "  // comment\n"
+   "  return \"critical\";\n"
+   "case log::info:return \"info\";\n"
+   "case log::warning: return \"warning\";\n"
+   "default:\n"
+   "  // comment\n"
+   "  return \"\";\n"
+   "case log::error:  return \"error\";\n"
+   "case log::severe: return \"severe\";\n"
+   "case log::extra_critical:\n"
+   "  // comment\n"
+   "  return \"extra critical\";\n"
+   "}",
+   Alignment);
+
+  Alignment.SpaceBeforeCaseColon = true;
+  verifyFormat("switch (level) {\n"
+   "case log::info :return \"info\";\n"
+   "case log::warning : return \"warning\";\n"
+   "default :   return \"default\";\n"
+   "}",
+   Alignment);
+  Alignment.SpaceBeforeCaseColon = false;
+
+  // Make sure we don't incorrectly align correctly across nested switch cases.
+  verifyFormat("switch (level) {\n"
+   "case log::info:return \"info\";\n"
+   "case log::warning: return \"warning\";\n"
+   "case log::other:\n"
+   "  switch (sublevel) {\n"
+   "  case log::info:return \"info\";\n"
+   "  case log::warning: return \"warning\";\n"
+   "  }\n"
+   "  break;\n"
+   "case log::error: return \"error\";\n"
+   "default: return \"default\";\n"
+   "}",
+   "switch (level) {\n"
+   "case log::info:return \"info\";\n"
+   "case log::warning: return \"warning\";\n"
+   "case log::other: switch (sublevel) {\n"
+   "  case log::info:return \"info\";\n"
+   "  case log::warning: return \"warning\";\n"
+   "}\n"
+   "break;\n"
+   "case log::error: return \"error\";\n"
+   "default: return \"default\";\n"
+   "}",
+   Alignment);
+
+  Alignment.AlignConsecutiveShortCaseStatements.AcrossEmptyLines = true;
+
+  verifyFormat("switch (level) {\n"
+   "case log::info:   

[PATCH] D151761: clang-format: Add AlignConsecutiveShortCaseStatements

[Björn Schäpers via Phabricator via cfe-commits]

HazardyKnusperkeks added inline comments.



Comment at: clang/unittests/Format/FormatTest.cpp:19217
+
+  // Verify comments and empty lines break the alignment
+  verifyFormat("switch (level) {\n"





Comment at: clang/unittests/Format/FormatTest.cpp:19218
+  // Verify comments and empty lines break the alignment
+  verifyFormat("switch (level) {\n"
+   "case log::info:return \"info\";\n"

If both strings are the same, you only need to supply it once.

Or are they different and I can't see it?



Comment at: clang/unittests/Format/FormatTest.cpp:19257
+
+  Alignment.ColumnLimit = 80;
+  Alignment.SpaceBeforeCaseColon = true;

For what is this?


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D151761/new/

https://reviews.llvm.org/D151761

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

[PATCH] D151761: clang-format: Add AlignConsecutiveShortCaseStatements

[Galen Elias via Phabricator via cfe-commits]

galenelias updated this revision to Diff 528072.
galenelias marked 3 inline comments as done.
galenelias retitled this revision from "clang-format: Add 
AlignConsecutiveShortCaseLabels" to "clang-format: Add 
AlignConsecutiveShortCaseStatements".
galenelias edited the summary of this revision.

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D151761/new/

https://reviews.llvm.org/D151761

Files:
  clang/docs/ClangFormatStyleOptions.rst
  clang/docs/ReleaseNotes.rst
  clang/include/clang/Format/Format.h
  clang/lib/Format/Format.cpp
  clang/lib/Format/WhitespaceManager.cpp
  clang/lib/Format/WhitespaceManager.h
  clang/unittests/Format/ConfigParseTest.cpp
  clang/unittests/Format/FormatTest.cpp

Index: clang/unittests/Format/FormatTest.cpp
===
--- clang/unittests/Format/FormatTest.cpp
+++ clang/unittests/Format/FormatTest.cpp
@@ -19192,6 +19192,148 @@
BracedAlign);
 }
 
+TEST_F(FormatTest, AlignConsecutiveShortCaseStatements) {
+  FormatStyle Alignment = getLLVMStyle();
+  Alignment.AllowShortCaseLabelsOnASingleLine = true;
+  Alignment.AlignConsecutiveShortCaseStatements.Enabled = true;
+  verifyFormat("switch (level) {\n"
+   "case log::info:return \"info\";\n"
+   "case log::warning: return \"warning\";\n"
+   "default:   return \"default\";\n"
+   "}",
+   Alignment);
+
+  verifyFormat("switch (level) {\n"
+   "case log::info:return \"info\";\n"
+   "case log::warning: return \"warning\";\n"
+   "}",
+   "switch (level) {\n"
+   "case log::info: return \"info\";\n"
+   "case log::warning:\n"
+   "  return \"warning\";\n"
+   "}",
+   Alignment);
+
+  // Verify comments and empty lines break the alignment
+  verifyFormat("switch (level) {\n"
+   "case log::info:return \"info\";\n"
+   "case log::warning: return \"warning\";\n"
+   "// comment\n"
+   "case log::critical: return \"critical\";\n"
+   "default:return \"default\";\n"
+   "\n"
+   "case log::severe: return \"severe\";\n"
+   "}",
+   "switch (level) {\n"
+   "case log::info:return \"info\";\n"
+   "case log::warning: return \"warning\";\n"
+   "// comment\n"
+   "case log::critical: return \"critical\";\n"
+   "default:return \"default\";\n"
+   "\n"
+   "case log::severe: return \"severe\";\n"
+   "}",
+   Alignment);
+
+  // Verify adjacent non-short case statements don't change the alignment, and
+  // properly break the set of consecutive statements.
+  verifyFormat("switch (level) {\n"
+   "case log::critical:\n"
+   "  // comment\n"
+   "  return \"critical\";\n"
+   "case log::info:return \"info\";\n"
+   "case log::warning: return \"warning\";\n"
+   "default:\n"
+   "  // comment\n"
+   "  return \"\";\n"
+   "case log::error:  return \"error\";\n"
+   "case log::severe: return \"severe\";\n"
+   "case log::extra_critical:\n"
+   "  // comment\n"
+   "  return \"extra critical\";\n"
+   "}",
+   Alignment);
+
+  Alignment.ColumnLimit = 80;
+  Alignment.SpaceBeforeCaseColon = true;
+  verifyFormat("switch (level) {\n"
+   "case log::info :return \"info\";\n"
+   "case log::warning : return \"warning\";\n"
+   "default :   return \"default\";\n"
+   "}",
+   Alignment);
+  Alignment.SpaceBeforeCaseColon = false;
+
+  // Make sure we don't incorrectly align correctly across nested switch cases.
+  verifyFormat("switch (level) {\n"
+   "case log::info:return \"info\";\n"
+   "case log::warning: return \"warning\";\n"
+   "case log::other:\n"
+   "  switch (sublevel) {\n"
+   "  case log::info:return \"info\";\n"
+   "  case log::warning: return \"warning\";\n"
+   "  }\n"
+   "  break;\n"
+   "case log::error: return \"error\";\n"
+   "default: return \"default\";\n"
+   "}",
+   "switch (level) {\n"
+   "case log::info:return \"info\";\n"
+   "case log::warning: return \"warning\";\n"
+   "case log::other: switch (sublevel) {\n"
+   "  case log::info:return \"info\";\n"
+   "  case log::warning: return \"warning\";\n"
+   "}\n"
+   "break;\n"
+   "case log::error: return \"error\";\n"
+   "default: