Page MenuHomePhabricator

[clang-format] Add SpaceBeforeColon option
ClosedPublic

Authored by Typz on Apr 26 2017, 2:35 AM.

Details

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

Diff Detail

Repository
rC Clang

Event Timeline

Typz created this revision.Apr 26 2017, 2:35 AM
Typz updated this revision to Diff 96695.Apr 26 2017, 2:48 AM

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

Typz added a comment.EditedApr 26 2017, 6:42 AM

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.

djasper edited edge metadata.May 17 2017, 6:09 AM

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 added a comment.May 18 2017, 7:03 AM

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) ?

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 added a comment.May 23 2017, 5:17 AM

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.

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

Typz added a comment.May 23 2017, 5:37 AM

Indeed, so this is why :-)

karies added a subscriber: karies.Jul 6 2017, 4:10 AM
Typz updated this revision to Diff 132963.Feb 6 2018, 2:56 AM

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.

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

karies added a comment.Feb 6 2018, 7:58 AM

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.

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 added a comment.Feb 6 2018, 9:27 AM

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.

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

include/clang/Format/Format.h
1528

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

unittests/Format/FormatTest.cpp
8953

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

Typz updated this revision to Diff 136299.Feb 28 2018, 8:32 AM
Typz marked 2 inline comments as done.

Address review comments.

djasper added inline comments.Feb 28 2018, 8:35 AM
unittests/Format/FormatTest.cpp
8969

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

8971

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

Typz updated this revision to Diff 136305.Feb 28 2018, 9:03 AM
Typz marked 2 inline comments as done.

Address review comments

unittests/Format/FormatTest.cpp
8969

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 accepted this revision.Feb 28 2018, 10:00 AM

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.

This revision is now accepted and ready to land.Feb 28 2018, 10:00 AM
This revision was automatically updated to reflect the committed changes.
This revision was automatically updated to reflect the committed changes.