[clang-format] fix Bug 38686: add AfterCaseLabel to BraceWrapping

[owenpan]

Summary

https://bugs.llvm.org/show_bug.cgi?id=38686

[owenpan]

Updated ClangFormatStyleOptions.rst.

[owenpan]

I'd greatly appreciate it if someone could review this before I commit it next week.

[JonasToth]

In D52527#1257277, @owenpan wrote:

I'd greatly appreciate it if someone could review this before I commit it next week.

Please do not commit without review. It is ok, to write ping every 5-7 days if there is no comment from the reviewers.

[owenpan]

ping

[krasimir]

OK, so this is not a real bug in the sense of non-working current features, it's more like a feature request.
As per https://clang.llvm.org/docs/ClangFormatStyleOptions.html#adding-additional-style-options, I (and I'm sure other reviewers) would like to understand better this use-case before it's added to clang-format.

[MyDeveloperDay]

@owenpan can I help you bring this back up to date and review, I think I saw another issue in the bugzilla requesting this.

I know the code owners like new option to have a style guide used by a large code base, but to me this seems a reasonable improvement

[owenpan]

@MyDeveloperDay: Can you add a link to the bugzilla report?

I stopped pushing my solution because as @krasimir said it's probably not a bug although the current documentation doesn't spell it out. Nonetheless, I'm with you and would like to see this improvement accepted.

[klimek]

Apart from the typo I think this is a simple enough change and a widely enough used style that it LG.

lib/Format/UnwrappedLineParser.cpp

181

Typo: WrapBrace?

[krasimir]

include/clang/Format/Format.h

638

This comment seems outdated (the one in ClangFormatStyleOptions.rst seems more recent).

The ClangFormatStyleOptions.rst file is automatically generated from this file. Please update this comment and regenerate ClangFormatStyleOptions.rst by using the clang/docs/tools/dump_format_style.py script.

lib/Format/Format.cpp

611

If Allman and GNU styles both follow the style suggested by this new flag, my mistake: this would be enough to add it to clang-format.

[MyDeveloperDay]

@ownenpan might be worth checking with @reuk who has been adding some options for the JUCE style guide and looking at the JUCE code it seems this style might match their style.

include/clang/Format/Format.h

638

@krasimir do you happen to know if this script is run by the build or is supposed to be run by the developer after making the change to Format.h

If the latter, then I reckon the two are out of sync, perhaps I should submit a change to realign them, but really there should be some sort of make target that lets us determine when they diverge otherwise they'll keep changing

[owenpan]

Thank you to all for reviewing this revision! Here is the update that addresses all of your comments.

(Also added @reuk to the reviewer list per @MyDeveloperDay 's suggestion.)

[owenpan]

@MyDeveloperDay :

do you happen to know if this script is run by the build or is supposed to be run by the developer after making the change to Format.h

I believe the developer must run it manually.

If the latter, then I reckon the two are out of sync, perhaps I should submit a change to realign them, but really there should be some sort of make target that lets us determine when they diverge otherwise they'll keep changing

+1

[owenpan]

llvm-svn: 357957

docs/ClangFormatStyleOptions.rst

.. code-block:: yaml
     BraceWrapping:
       AfterEnum: true
       AfterStruct: false
       SplitEmptyFunction: false
 
   Nested configuration flags:
 
 
+  * ``bool AfterCaseLabel`` Wrap case labels.
+
+    .. code-block:: c++
+
+      false:                                true:
+      switch (foo) {                vs.     switch (foo) {
+        case 1: {                              case 1:
+          bar();                               {
+          break;                                 bar();
+        }                                        break;
+        default: {                             }
+          plop();                              default:
+        }                                      {
+      }                                          plop();
+                                               }
+                                             }
+
   * ``bool AfterClass`` Wrap class definitions.
 
     .. code-block:: c++
 
       true:
       class foo {};
 
       false:

include/clang/Format/Format.h

struct FormatStyle {
   /// Precise control over the wrapping of braces.
   /// \code
   ///   # Should be declared this way:
   ///   BreakBeforeBraces: Custom
   ///   BraceWrapping:
   ///       AfterClass: true
   /// \endcode
   struct BraceWrappingFlags {
+    /// Wrap case and default blocks.
+    /// \code
+    ///   true:
+    ///   case foo:
+    ///   {
+    ///     bar();
+    ///   }
+    ///
+    ///   false:
+    ///   default: {
+    ///     foo();
+    ///   }
+    /// \endcode
+    bool AfterCaseLabel;
     /// Wrap class definitions.
     /// \code
     ///   true:
     ///   class foo {};
     ///
     ///   false:
     ///   class foo
     ///   {};

lib/Format/Format.cpp

static void mapping(IO &IO, FormatStyle &Style) {
     IO.mapOptional("Standard", Style.Standard);
     IO.mapOptional("TabWidth", Style.TabWidth);
     IO.mapOptional("UseTab", Style.UseTab);
   }
 };
 
 template <> struct MappingTraits<FormatStyle::BraceWrappingFlags> {
   static void mapping(IO &IO, FormatStyle::BraceWrappingFlags &Wrapping) {
+    IO.mapOptional("AfterCaseLabel", Wrapping.AfterCaseLabel);
     IO.mapOptional("AfterClass", Wrapping.AfterClass);
     IO.mapOptional("AfterControlStatement", Wrapping.AfterControlStatement);
     IO.mapOptional("AfterEnum", Wrapping.AfterEnum);
     IO.mapOptional("AfterFunction", Wrapping.AfterFunction);
     IO.mapOptional("AfterNamespace", Wrapping.AfterNamespace);
     IO.mapOptional("AfterObjCDeclaration", Wrapping.AfterObjCDeclaration);
     IO.mapOptional("AfterStruct", Wrapping.AfterStruct);
     IO.mapOptional("AfterUnion", Wrapping.AfterUnion);
std::string ParseErrorCategory::message(int EV) const {
   }
   llvm_unreachable("unexpected parse error");
 }
 
 static FormatStyle expandPresets(const FormatStyle &Style) {
   if (Style.BreakBeforeBraces == FormatStyle::BS_Custom)
     return Style;
   FormatStyle Expanded = Style;
-  Expanded.BraceWrapping = {false, false, false, false, false,
+  Expanded.BraceWrapping = {false, false, false, false, false, false,
                             false, false, false, false, false,
                             false, false, true,  true,  true};
   switch (Style.BreakBeforeBraces) {
   case FormatStyle::BS_Linux:
     Expanded.BraceWrapping.AfterClass = true;
     Expanded.BraceWrapping.AfterFunction = true;
     Expanded.BraceWrapping.AfterNamespace = true;
     break;
   case FormatStyle::BS_Mozilla:
     Expanded.BraceWrapping.AfterClass = true;
     Expanded.BraceWrapping.AfterEnum = true;
     Expanded.BraceWrapping.AfterFunction = true;
     Expanded.BraceWrapping.AfterStruct = true;
     Expanded.BraceWrapping.AfterUnion = true;
     Expanded.BraceWrapping.AfterExternBlock = true;
     Expanded.BraceWrapping.SplitEmptyFunction = true;
     Expanded.BraceWrapping.SplitEmptyRecord = false;
     break;
   case FormatStyle::BS_Stroustrup:
     Expanded.BraceWrapping.AfterFunction = true;
     Expanded.BraceWrapping.BeforeCatch = true;
     Expanded.BraceWrapping.BeforeElse = true;
     break;
   case FormatStyle::BS_Allman:
+    Expanded.BraceWrapping.AfterCaseLabel = true;
     Expanded.BraceWrapping.AfterClass = true;
     Expanded.BraceWrapping.AfterControlStatement = true;
     Expanded.BraceWrapping.AfterEnum = true;
     Expanded.BraceWrapping.AfterFunction = true;
     Expanded.BraceWrapping.AfterNamespace = true;
     Expanded.BraceWrapping.AfterObjCDeclaration = true;
     Expanded.BraceWrapping.AfterStruct = true;
     Expanded.BraceWrapping.AfterExternBlock = true;
     Expanded.BraceWrapping.BeforeCatch = true;
     Expanded.BraceWrapping.BeforeElse = true;
     break;
   case FormatStyle::BS_GNU:
     Expanded.BraceWrapping = {true, true, true, true, true, true, true, true,
-                              true, true, true, true, true, true, true};
+                              true, true, true, true, true, true, true, true};
     break;
   case FormatStyle::BS_WebKit:
     Expanded.BraceWrapping.AfterFunction = true;
     break;
   default:
     break;
   }
   return Expanded;
FormatStyle getLLVMStyle() {
   LLVMStyle.AlwaysBreakAfterDefinitionReturnType = FormatStyle::DRTBS_None;
   LLVMStyle.AlwaysBreakBeforeMultilineStrings = false;
   LLVMStyle.AlwaysBreakTemplateDeclarations = FormatStyle::BTDS_MultiLine;
   LLVMStyle.BinPackArguments = true;
   LLVMStyle.BinPackParameters = true;
   LLVMStyle.BreakBeforeBinaryOperators = FormatStyle::BOS_None;
   LLVMStyle.BreakBeforeTernaryOperators = true;
   LLVMStyle.BreakBeforeBraces = FormatStyle::BS_Attach;
-  LLVMStyle.BraceWrapping = {false, false, false, false, false,
+  LLVMStyle.BraceWrapping = {false, false, false, false, false, false,
                              false, false, false, false, false,
                              false, false, true,  true,  true};
   LLVMStyle.BreakAfterJavaFieldAnnotations = false;
   LLVMStyle.BreakConstructorInitializers = FormatStyle::BCIS_BeforeColon;
   LLVMStyle.BreakInheritanceList = FormatStyle::BILS_BeforeColon;
   LLVMStyle.BreakStringLiterals = true;
   LLVMStyle.ColumnLimit = 80;
   LLVMStyle.CommentPragmas = "^ IWYU pragma:";

lib/Format/UnwrappedLineParser.cpp

private:
   std::unique_ptr<UnwrappedLine> PreBlockLine;
   SmallVectorImpl<UnwrappedLine> *OriginalLines;
 };
 
 class CompoundStatementIndenter {
 public:
   CompoundStatementIndenter(UnwrappedLineParser *Parser,
                             const FormatStyle &Style, unsigned &LineLevel)
+      : CompoundStatementIndenter(Parser, LineLevel,
+                                  Style.BraceWrapping.AfterControlStatement,
+                                  Style.BraceWrapping.IndentBraces) {
+  }
+  CompoundStatementIndenter(UnwrappedLineParser *Parser, unsigned &LineLevel,
+                            bool WrapeBrace, bool IndentBrace)
       : LineLevel(LineLevel), OldLineLevel(LineLevel) {
-    if (Style.BraceWrapping.AfterControlStatement)
+    if (WrapeBrace)
       Parser->addUnwrappedLine();
-    if (Style.BraceWrapping.IndentBraces)
+    if (IndentBrace)
       ++LineLevel;
   }
   ~CompoundStatementIndenter() { LineLevel = OldLineLevel; }
 
 private:
   unsigned &LineLevel;
   unsigned OldLineLevel;
 };

 }
 
 void UnwrappedLineParser::parseLabel() {
   nextToken();
   unsigned OldLineLevel = Line->Level;
   if (Line->Level > 1 || (!Line->InPPDirective && Line->Level > 0))
     --Line->Level;
   if (CommentsBeforeNextToken.empty() && FormatTok->Tok.is(tok::l_brace)) {
-    CompoundStatementIndenter Indenter(this, Style, Line->Level);
+    CompoundStatementIndenter Indenter(this, Line->Level,
+                                       Style.BraceWrapping.AfterCaseLabel,
+                                       Style.BraceWrapping.IndentBraces);
     parseBlock(/*MustBeDeclaration=*/false);
     if (FormatTok->Tok.is(tok::kw_break)) {
       if (Style.BraceWrapping.AfterControlStatement)
         addUnwrappedLine();
       parseStructuralElement();
     }
     addUnwrappedLine();
   } else {

unittests/Format/FormatTest.cpp

verifyFormat("switch (a) {\n"
                "  return;\n"
                "}",
                getLLVMStyleWithColumns(34));
 
   FormatStyle Style = getLLVMStyle();
   Style.IndentCaseLabels = true;
   Style.AllowShortBlocksOnASingleLine = false;
   Style.BreakBeforeBraces = FormatStyle::BS_Custom;
+  Style.BraceWrapping.AfterCaseLabel = true;
   Style.BraceWrapping.AfterControlStatement = true;
   EXPECT_EQ("switch (n)\n"
             "{\n"
             "  case 0:\n"
             "  {\n"
             "    return false;\n"
             "  }\n"
             "  default:\n"
             "  {\n"
             "    return true;\n"
             "  }\n"
             "}",
             format("switch (n) {\n"
                    "  case 0: {\n"
                    "    return false;\n"
                    "  }\n"
                    "  default: {\n"
                    "    return true;\n"
                    "  }\n"
                    "}",
                    Style));
+  Style.BraceWrapping.AfterCaseLabel = false;
+  EXPECT_EQ("switch (n)\n"
+            "{\n"
+            "  case 0: {\n"
+            "    return false;\n"
+            "  }\n"
+            "  default: {\n"
+            "    return true;\n"
+            "  }\n"
+            "}",
+            format("switch (n) {\n"
+                   "  case 0:\n"
+                   "  {\n"
+                   "    return false;\n"
+                   "  }\n"
+                   "  default:\n"
+                   "  {\n"
+                   "    return true;\n"
+                   "  }\n"
+                   "}",
+                   Style));
 }
 
 TEST_F(FormatTest, CaseRanges) {
   verifyFormat("switch (x) {\n"
                "case 'A' ... 'Z':\n"
                "case 1 ... 5:\n"
                "case a ... b:\n"
                "  break;\n"
EXPECT_EQ("switch (n) {\n"
                    "default/*comments*/:\n"
                    "  return true;\n"
                    "case 0:\n"
                    "  return false;\n"
                    "}",
                    Style));
   Style.AllowShortCaseLabelsOnASingleLine = true;
   Style.BreakBeforeBraces = FormatStyle::BS_Custom;
+  Style.BraceWrapping.AfterCaseLabel = true;
   Style.BraceWrapping.AfterControlStatement = true;
   EXPECT_EQ("switch (n)\n"
             "{\n"
             "  case 0:\n"
             "  {\n"
             "    return false;\n"
             "  }\n"
             "  default:\n"
TEST_F(FormatTest, ParsesConfigurationBools) {
   CHECK_PARSE_BOOL(SpaceAfterCStyleCast);
   CHECK_PARSE_BOOL(SpaceAfterTemplateKeyword);
   CHECK_PARSE_BOOL(SpaceBeforeAssignmentOperators);
   CHECK_PARSE_BOOL(SpaceBeforeCpp11BracedList);
   CHECK_PARSE_BOOL(SpaceBeforeCtorInitializerColon);
   CHECK_PARSE_BOOL(SpaceBeforeInheritanceColon);
   CHECK_PARSE_BOOL(SpaceBeforeRangeBasedForLoopColon);
 
+  CHECK_PARSE_NESTED_BOOL(BraceWrapping, AfterCaseLabel);
   CHECK_PARSE_NESTED_BOOL(BraceWrapping, AfterClass);
   CHECK_PARSE_NESTED_BOOL(BraceWrapping, AfterControlStatement);
   CHECK_PARSE_NESTED_BOOL(BraceWrapping, AfterEnum);
   CHECK_PARSE_NESTED_BOOL(BraceWrapping, AfterFunction);
   CHECK_PARSE_NESTED_BOOL(BraceWrapping, AfterNamespace);
   CHECK_PARSE_NESTED_BOOL(BraceWrapping, AfterObjCDeclaration);
   CHECK_PARSE_NESTED_BOOL(BraceWrapping, AfterStruct);
   CHECK_PARSE_NESTED_BOOL(BraceWrapping, AfterUnion);