[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 ↗

(On Diff #167225)

Typo: WrapBrace?

[krasimir]

include/clang/Format/Format.h

638 ↗

(On Diff #167225)

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 ↗

(On Diff #167225)

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 ↗

(On Diff #167225)

@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

clang/docs/ClangFormatStyleOptions.rst

* ``SFS_All`` (in configuration: ``All``)
 
     .. code-block:: c++
 
       class Foo {
         void f() { foo(); }
       };
       void f() { bar(); }
 
+
+
 **AllowShortIfStatementsOnASingleLine** (``ShortIfStyle``)
-  Dependent on the value, ``if (a) return 0;`` can be put on a
-  single line.
+  If ``true``, ``if (a) return;`` can be put on a single line.
 
   Possible values:
 
   * ``SIS_Never`` (in configuration: ``Never``)
-    Do not allow short if functions.
+    Never put short ifs on the same line.
 
     .. code-block:: c++
 
-       if (a)
-         return;
-       else
-         return;
+        return ;
+      else {
+      }
 
   * ``SIS_WithoutElse`` (in configuration: ``WithoutElse``)
-    Allow short if functions on the same line, as long as else
-    is not a compound statement.
+    Without else put short ifs on the same line only if
+    the else is not a compound statement.
 
     .. code-block:: c++
 
-       if (a) return;
-       else
-         return;
 
-       if (a)
-         return;
-       else {
-         return;
-       }
-
   * ``SIS_Always`` (in configuration: ``Always``)
-    Allow short if statements even if the else is a compound statement.
+    Always put short ifs on the same line if
+    the else is not a compound statement or not.
 
     .. code-block:: c++
 
-       if (a) return;
-       else {
-          return;
-       }
 
+
+
 **AllowShortLambdasOnASingleLine** (``ShortLambdaStyle``)
   Dependent on the value, ``auto lambda []() { return 0; }`` can be put on a
   single line.
 
   Possible values:
 
   * ``SLS_None`` (in configuration: ``None``)
     Never merge lambdas into a single line.
.. 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:
.. code-block:: c++
       }
       enum X : int { A, B };
 
   * ``BS_Allman`` (in configuration: ``Allman``)
     Always break before braces.
 
     .. code-block:: c++
 
-      try
-      {
+      try {
         foo();
       }
-      catch ()
-      {
+      catch () {
       }
       void foo() { bar(); }
-      class foo
-      {
+      class foo {
       };
-      if (foo())
-      {
+      if (foo()) {
       }
-      else
-      {
+      else {
       }
-      enum X : int
-      {
-        A,
-        B
-      };
+      enum X : int { A, B };
 
   * ``BS_GNU`` (in configuration: ``GNU``)
     Always break before braces and add an extra level of indentation to
     braces of control statements, not to those of class, function
     or other definitions.
 
     .. code-block:: c++
 
.. code-block:: c++
 
        #if FOO
          #if BAR
            #include <foo>
          #endif
        #endif
 
 
+
 **IndentWidth** (``unsigned``)
   The number of columns to use for indentation.
 
   .. code-block:: c++
 
      IndentWidth: 3
 
      void f() {
**SpaceAfterCStyleCast** (``bool``)
 
   .. code-block:: c++
 
      true:                                  false:
      (int) i;                       vs.     (int)i;
 
 **SpaceAfterLogicalNot** (``bool``)
   If ``true``, a space is inserted after the logical not operator (``!``).
+
   .. code-block:: c++
 
      true:                                  false:
      ! someExpression();            vs.     !someExpression();
 
 **SpaceAfterTemplateKeyword** (``bool``)
   If ``true``, a space will be inserted after the 'template' keyword.
 
* ``SBPO_ControlStatements`` (in configuration: ``ControlStatements``)
     .. code-block:: c++
 
        void f() {
          if (true) {
            f();
          }
        }
 
+  * ``SBPO_NonEmptyParentheses`` (in configuration: ``NonEmptyParentheses``)
+    Put a space before opening parentheses only if the parentheses are not
+    empty i.e. '()'
+
+    .. code-block:: c++
+
+      void() {
+        if (true) {
+          f();
+          g (x, y, z);
+        }
+      }
+
   * ``SBPO_Always`` (in configuration: ``Always``)
     Always put a space before opening parentheses, except when it's
     prohibited by the syntax rules (in function-like macro definitions) or
     when determined by other style rules (after unary operators, opening
     parentheses, etc.)
 
     .. code-block:: c++
 

clang/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 labels.
+    /// \code
+    ///   false:                                true:
+    ///   switch (foo) {                vs.     switch (foo) {
+    ///     case 1: {                             case 1:
+    ///       bar();                              {
+    ///       break;                                bar();
+    ///     }                                       break;
+    ///     default: {                            }
+    ///       plop();                             default:
+    ///     }                                     {
+    ///   }                                         plop();
+    ///                                           }
+    ///                                         }
+    /// \endcode
+    bool AfterCaseLabel;
     /// Wrap class definitions.
     /// \code
     ///   true:
     ///   class foo {};
     ///
     ///   false:
     ///   class foo
     ///   {};

clang/lib/Format/Format.cpp

static void mapping(IO &IO, FormatStyle &Style) {
     IO.mapOptional("StatementMacros", Style.StatementMacros);
     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(FormatStyle::LanguageKind Language) {
   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:";

clang/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 WrapBrace, bool IndentBrace)
       : LineLevel(LineLevel), OldLineLevel(LineLevel) {
-    if (Style.BraceWrapping.AfterControlStatement)
+    if (WrapBrace)
       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 {

clang/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);
TEST_F(FormatTest, ConfigurableContinuationIndentWidth) {
   EXPECT_EQ("int i =\n"
             "      longFunction(\n"
             "            arg);",
             format("int i = longFunction(arg);", SixIndent));
 }
 
 TEST_F(FormatTest, WrappedClosingParenthesisIndent) {
   FormatStyle Style = getLLVMStyle();
-  verifyFormat(
-      "int Foo::getter(\n"
+  verifyFormat("int Foo::getter(\n"
                "    //\n"
                ") const {\n"
                "  return foo;\n"
                "}",
                Style);
-  verifyFormat(
-      "void Foo::setter(\n"
+  verifyFormat("void Foo::setter(\n"
                "    //\n"
                ") {\n"
                "  foo = 1;\n"
                "}",
                Style);
 }
 
 TEST_F(FormatTest, SpacesInAngles) {
   FormatStyle Spaces = getLLVMStyle();
   Spaces.SpacesInAngles = true;
 
   verifyFormat("static_cast< int >(arg);", Spaces);
   verifyFormat("template < typename T0, typename T1 > void f() {}", Spaces);