[clang-format] Add SpaceBeforeColon option

[Typz]

Summary

When disabled, this option allows removing the space before colon,
making it act more like the semi-colon. When enabled (default), the
current behavior is not affected.

This mostly affects C++11 loop, initializer list, inheritance list and
container literals:

class Foo: Bar {}
Foo::Foo(): a(a) {}
for (auto i: myList) {}
f({a: 1, b: 2, c: 3});

[Typz]

Fix ternary operator handling: must not be affected by this option.

[Typz]

This option is used in the open source projects which use Applied Informatics' coding styles: https://www.appinf.com/download/CppCodingStyleGuide.pdf, like Poco.
And I am indeed willing to maintain the patch.

[Typz]

ping?

[djasper]

I have looked through the PDF you linked to, but I couldn't really find much about formatting. I have found one instance of a constructor initializer list, but there is no accompanying text saying that this is even intentional. Haven't found a range-based for loop. As such, I am not convinced that this option carries it's weight.

Also see my comment. It's very hard to even name this option without it being confusing to users.

[Typz]

I checked the code of POCO, and it indeed follows this convention (though there does not seem to be any C++11 for loop indeed).
We also use this style at our company.

Also see my comment.

I could not find your comment... can you please re-post?

It's very hard to even name this option without it being confusing to users.

What do you mean? 'space before colon' seems quite explicit to me... The only thing I can think of is that it does not apply to ternary operator and labels (though I don't think that would be misleading for user); do you think it should be a flag with different modes, similar to BreakBeforeBinaryOperators (e.g. Never, LabelsAndOperators, Always) ?

[djasper]

Hm, can't really remember what I meant by "my comment". Probably not important.

So, I still see two problems:

• I would not count the link you mentioned as a publicly accessible style guide.
• I don't think the naming and granularity of this option is right.

You specifically want to control two things. The space in front of constructor initializers colons and of range-based for loop colons. Then you also (accidentally?) change the spacing in dict literals. However, the latter is already controlled by SpacesInContainerLiterals. So there even seems to be a conflict here. You do not want to change anything for ternary expressions or labels. Oh, and there are also colons in inline assembly. Colons are probably the most overloaded tokens.

Now, we could of course allow much more fine-grained control over colons, but really, that adds quite a bit of complexity to the clang-format configuration. And I am not sure whether the added costs (both in maintenance of clang-format and discoverability for users) are covered by the very small added value of that removed space for you.

I am sorry I don't have to better answer, but I have to weigh off the benefit this adds for you vs. the reduced usefulness to all other users of clang-format. In general, we'd rather have clang-format have fewer configuration options and support them really well. For now, I am against moving forward with this patch, unless we find a good way to structure the options and a publicly accessible coding style that really supports this (explicitly, not just in an example somewhere that's actually meant to show something else).

[Typz]

In D32525#760710, @djasper wrote:

Then you also (accidentally?) change the spacing in dict literals. However, the latter is already controlled by SpacesInContainerLiterals.

not really accidentally: I want the space on the 'outside' of dict literals, but not before the the colons. Something like:

[ a: 1, b: 2 ]
f({ a: 1, b: 2, c: 3 });

(btw this option does not seem to affect the space on the 'outside' of dict literals with curly braces. Is this a bug/side effect, or an actual expected behavior? This specific case would also happen for C99 designated literals)

The "logic" behind this patch is that it affects all cases where the colon is used as a separator, but not as a label or operator: e.g. where it would make some sense to format it similarly to a semi-colon. I am not sure where the assembly stands in this regard, though.

[djasper]

Don't C99 designated literals use "=" instead of ":"?

[Typz]

Indeed, so this is why :-)

[Typz]

Split the option into 3 separate options: SpaceBeforeCtorInitializerColon, SpaceBeforeInheritanceColon and SpaceBeforeRangeBasedForLoopColon.
This makes each option clearer and more consistent, with no ambiguities due to interractions with other options.

[djasper]

You still haven't addressed my comment about there not being a publicly accessible style guide recommending these.

[karies]

Does this qualify? https://github.com/root-project/root/blob/master/.clang-format#L84

# You want this : enable it if you have https://reviews.llvm.org/D32525
# SpaceBeforeColon: false

in ROOT's .clang-format.

[djasper]

No. The reason for us generally asking for a style guide is because it unambiguously clarifies the exact style that is to be preferred. Projects that don't have a style guide written down also often do not agree on what the style should be.

That said, I think the style options here are so straightforward now that I think I'd be ok if there isn't one.

[Typz]

Indeed, I have yet find more precisely documented coding rules which require this format, but I thought I could at least address the non-precise aspect of the patch itself in the mean-time.

[Typz]

ping?

[djasper]

I think this generally looks good, but needs a few more tests.

include/clang/Format/Format.h

1204

When this file is changed, can you also run docs/tools/dump_format_style.py to update the docs?

unittests/Format/FormatTest.cpp

7539

Can you add tests for the other values of BreakConstructorInitializers (BCIS_BeforeColon, BCIS_BeforeComma)?

[Typz]

Address review comments.

[djasper]

unittests/Format/FormatTest.cpp

7555

This is a useless test if it doesn't actually have multiple initializers separated by a comma.

7557

Has this been formatted by clang-format? I think it'd break after the comma.

[Typz]

Address review comments

unittests/Format/FormatTest.cpp

7555

since we are interested only in the behavior next to the colon, I think it is relevant... But I'll add extra parameter to make it more realistic,

[djasper]

Ah, I thought it was somehow possible to create:

Constructor(): aaaaaa()
             , bbbbbb() {},

but I guess clang-format always inserts a break there. Sorry for chasing you in circles.

include/clang/Format/Format.h

   /// \brief If ``false``, spaces will be removed before assignment operators.
   /// \code
   ///    true:                                  false:
   ///    int a = 5;                     vs.     int a=5;
   ///    a += 42                                a+=42;
   /// \endcode
   bool SpaceBeforeAssignmentOperators;
 
+  /// \brief If ``false``, spaces will be removed before colon.
+  /// This affects C++11 loop, initializer list, inheritance list and container
+  /// literals.
+  /// \code
+  ///    true:                                  false:
+  ///    class Foo : Bar {}             vs.     class Foo: Bar {}
+  ///    Foo::Foo() : a(a) {}                   Foo::Foo(): a(a) {}
+  ///    for (auto i : myList) {}               for (auto i: myList) {}
+  ///    f({a : 1, b : 2, c : 3});              f({a: 1, b: 2, c: 3});
+  /// \endcode
+  bool SpaceBeforeColon;
+
   /// \brief Different ways to put a space before opening parentheses.
   enum SpaceBeforeParensOptions {
     /// Never put a space before opening parentheses.
     /// \code
     ///    void f() {
     ///      if(true) {
     ///        f();
     ///      }
return AccessModifierOffset == R.AccessModifierOffset &&
            PenaltyBreakFirstLessLess == R.PenaltyBreakFirstLessLess &&
            PenaltyBreakString == R.PenaltyBreakString &&
            PenaltyExcessCharacter == R.PenaltyExcessCharacter &&
            PenaltyReturnTypeOnItsOwnLine == R.PenaltyReturnTypeOnItsOwnLine &&
            PointerAlignment == R.PointerAlignment &&
            SpaceAfterCStyleCast == R.SpaceAfterCStyleCast &&
            SpaceAfterTemplateKeyword == R.SpaceAfterTemplateKeyword &&
            SpaceBeforeAssignmentOperators == R.SpaceBeforeAssignmentOperators &&
+           SpaceBeforeColon == R.SpaceBeforeColon &&
            SpaceBeforeParens == R.SpaceBeforeParens &&
            SpaceInEmptyParentheses == R.SpaceInEmptyParentheses &&
            SpacesBeforeTrailingComments == R.SpacesBeforeTrailingComments &&
            SpacesInAngles == R.SpacesInAngles &&
            SpacesInContainerLiterals == R.SpacesInContainerLiterals &&
            SpacesInCStyleCastParentheses == R.SpacesInCStyleCastParentheses &&
            SpacesInParentheses == R.SpacesInParentheses &&
            SpacesInSquareBrackets == R.SpacesInSquareBrackets &&

lib/Format/Format.cpp

IO.mapOptional("PenaltyReturnTypeOnItsOwnLine",
                    Style.PenaltyReturnTypeOnItsOwnLine);
     IO.mapOptional("PointerAlignment", Style.PointerAlignment);
     IO.mapOptional("ReflowComments", Style.ReflowComments);
     IO.mapOptional("SortIncludes", Style.SortIncludes);
     IO.mapOptional("SpaceAfterCStyleCast", Style.SpaceAfterCStyleCast);
     IO.mapOptional("SpaceAfterTemplateKeyword", Style.SpaceAfterTemplateKeyword);
     IO.mapOptional("SpaceBeforeAssignmentOperators",
                    Style.SpaceBeforeAssignmentOperators);
+    IO.mapOptional("SpaceBeforeColon", Style.SpaceBeforeColon);
     IO.mapOptional("SpaceBeforeParens", Style.SpaceBeforeParens);
     IO.mapOptional("SpaceInEmptyParentheses", Style.SpaceInEmptyParentheses);
     IO.mapOptional("SpacesBeforeTrailingComments",
                    Style.SpacesBeforeTrailingComments);
     IO.mapOptional("SpacesInAngles", Style.SpacesInAngles);
     IO.mapOptional("SpacesInContainerLiterals",
                    Style.SpacesInContainerLiterals);
     IO.mapOptional("SpacesInCStyleCastParentheses",
FormatStyle getLLVMStyle() {
   LLVMStyle.ReflowComments = true;
   LLVMStyle.SpacesInParentheses = false;
   LLVMStyle.SpacesInSquareBrackets = false;
   LLVMStyle.SpaceInEmptyParentheses = false;
   LLVMStyle.SpacesInContainerLiterals = true;
   LLVMStyle.SpacesInCStyleCastParentheses = false;
   LLVMStyle.SpaceAfterCStyleCast = false;
   LLVMStyle.SpaceAfterTemplateKeyword = true;
+  LLVMStyle.SpaceBeforeColon = true;
   LLVMStyle.SpaceBeforeParens = FormatStyle::SBPO_ControlStatements;
   LLVMStyle.SpaceBeforeAssignmentOperators = true;
   LLVMStyle.SpacesInAngles = false;
 
   LLVMStyle.PenaltyBreakComment = 300;
   LLVMStyle.PenaltyBreakFirstLessLess = 120;
   LLVMStyle.PenaltyBreakString = 1000;
   LLVMStyle.PenaltyExcessCharacter = 1000000;

lib/Format/TokenAnnotator.cpp

if (Right.isOneOf(TT_TrailingReturnArrow, TT_LambdaArrow) ||
     return true;
   if (Right.is(TT_OverloadedOperatorLParen))
     return Style.SpaceBeforeParens == FormatStyle::SBPO_Always;
   if (Left.is(tok::comma))
     return true;
   if (Right.is(tok::comma))
     return false;
   if (Right.isOneOf(TT_CtorInitializerColon, TT_ObjCBlockLParen))
-    return true;
+    return Style.SpaceBeforeColon;
   if (Right.is(tok::colon)) {
+    if (!Style.SpaceBeforeColon)
+      return false;
     if (Line.First->isOneOf(tok::kw_case, tok::kw_default) ||
         !Right.getNextNonComment() || Right.getNextNonComment()->is(tok::semi))
       return false;
     if (Right.is(TT_ObjCMethodExpr))
       return false;
     if (Left.is(tok::question))
       return false;
     if (Right.is(TT_InlineASMColon) && Left.is(tok::coloncolon))

unittests/Format/FormatTest.cpp

TEST_F(FormatTest, ConfigurableSpaceBeforeAssignmentOperators) {
 
   FormatStyle Spaces = getLLVMStyle();
   Spaces.SpaceBeforeAssignmentOperators = false;
   verifyFormat("int a= 5;", Spaces);
   verifyFormat("a+= 42;", Spaces);
   verifyFormat("a or_eq 8;", Spaces);
 }
 
+TEST_F(FormatTest, ConfigurableSpaceBeforeColon) {
+  verifyFormat("class Foo : public Bar {};");
+  verifyFormat("Foo::Foo() : foo(1) {}");
+  verifyFormat("for (auto a : b) {\n}");
+
+  FormatStyle Spaces = getLLVMStyle();
+  Spaces.SpaceBeforeColon = false;
+  verifyFormat("class Foo: public Bar {};", Spaces);
+  verifyFormat("Foo::Foo(): foo(1) {}", Spaces);
+  verifyFormat("for (auto a: b) {\n}", Spaces);
+}
+
 TEST_F(FormatTest, AlignConsecutiveAssignments) {
   FormatStyle Alignment = getLLVMStyle();
   Alignment.AlignConsecutiveAssignments = false;
   verifyFormat("int a = 5;\n"
                "int oneTwoThree = 123;",
                Alignment);
   verifyFormat("int a = 5;\n"
                "int oneTwoThree = 123;",
                Alignment);
 
   Alignment.AlignConsecutiveAssignments = true;
   verifyFormat("int a           = 5;\n"
                "int oneTwoThree = 123;",
                Alignment);
   verifyFormat("int a           = method();\n"
                "int oneTwoThree = 133;",
                Alignment);
   verifyFormat("a &= 5;\n"
                "bcd *= 5;\n"
                "ghtyf += 5;\n"
                "dvfvdb -= 5;\n"
                "a /= 5;\n"
                "vdsvsv %= 5;\n"
                "sfdbddfbdfbb ^= 5;\n"
                "dvsdsv |= 5;\n"
                "int dsvvdvsdvvv = 123;",
                Alignment);
   verifyFormat("int i = 1, j = 10;\n"
                "something = 2000;",
                Alignment);
   verifyFormat("something = 2000;\n"
                "int i = 1, j = 10;\n",
TEST_F(FormatTest, ParsesConfigurationBools) {
   CHECK_PARSE_BOOL(SpacesInSquareBrackets);
   CHECK_PARSE_BOOL(SpacesInAngles);
   CHECK_PARSE_BOOL(SpaceInEmptyParentheses);
   CHECK_PARSE_BOOL(SpacesInContainerLiterals);
   CHECK_PARSE_BOOL(SpacesInCStyleCastParentheses);
   CHECK_PARSE_BOOL(SpaceAfterCStyleCast);
   CHECK_PARSE_BOOL(SpaceAfterTemplateKeyword);
   CHECK_PARSE_BOOL(SpaceBeforeAssignmentOperators);
+  CHECK_PARSE_BOOL(SpaceBeforeColon);
 
   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);