This is an archive of the discontinued LLVM Phabricator instance.

[clang-format] Add SpaceBeforeCpp11BracedList option.
ClosedPublic

Authored by rkirsling on Apr 24 2018, 10:56 AM.

Details

Summary

WebKit C++ style for object initialization is as follows:

Foo foo { bar };

Yet using clang-format -style=webkit changes this to:

Foo foo{ bar };

As there is no existing combination of rules that will ensure a space before a braced list in this fashion, this patch adds a new SpaceBeforeCpp11BracedList rule.

Diff Detail

Repository
rC Clang

Event Timeline

rkirsling created this revision.Apr 24 2018, 10:56 AM

Is this written down somewhere? https://webkit.org/code-style-guidelines/ doesn't seem to mention it.

Is this written down somewhere? https://webkit.org/code-style-guidelines/ doesn't seem to mention it.

I agree that it really ought to be mentioned there—I'll try to bring that up with the original authors of that document.

It is enforced by the style checker (though the rule is not specific to braced initialization):
https://github.com/WebKit/webkit/blob/master/Tools/Scripts/webkitpy/style/checkers/cpp.py#L1972-L1978

Statistically speaking, when grepping the Source directory (excluding Source/ThirdParty) for *.h and *.cpp files:

  • with space (\w+\s\{.*\};) has 11544 matches across 2744 files
  • without space (\w+\{.*\};) has 29 matches across 10 files

Guidelines page has been updated (https://trac.webkit.org/changeset/231085), though it may take a bit for the website to update.

Rule has been published:
https://webkit.org/code-style-guidelines/#spacing-braced-init

Hopefully that suffices for motivation. :)

jfb accepted this revision.May 1 2018, 2:58 PM
jfb added a subscriber: jfb.

On the WebKit side this lgtm. Let's leave some time for clang-format folks to chime in.

This revision is now accepted and ready to land.May 1 2018, 2:58 PM

Any further commentary? :)

Updated patch to resolve conflict and include full diff context.

If there are no objections to this change, may I request a commit based on the approval that @jfb provided?

@klimek In our IRC discussion yesterday, I know you expressed disapproval of WebKit's choice, but given its reality, am I correct in concluding that this can be landed?

rkirsling updated this revision to Diff 149395.May 31 2018, 9:29 PM

Resolved another rebase conflict to keep this patch mergeable.

FWIW, please note that this space-before-brace style is not specific to WebKit; CppCoreGuidelines exhibits it as well:
http://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#es23-prefer-the--initializer-syntax

jfb added a comment.Jun 4 2018, 11:02 AM

FWIW, please note that this space-before-brace style is not specific to WebKit; CppCoreGuidelines exhibits it as well:
http://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#es23-prefer-the--initializer-syntax

The main point of clang-format, for me , is to not argue about style. Can we... not argue about WebKit's and others' choice? :)

hans added a subscriber: hans.Jun 12 2018, 2:00 AM

FWIW, please note that this space-before-brace style is not specific to WebKit; CppCoreGuidelines exhibits it as well:
http://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#es23-prefer-the--initializer-syntax

This and WebKit's style seem like compelling arguments to support this option.

klimek, djasper: Do you have any objections against landing this?

klimek accepted this revision.Jun 12 2018, 10:27 AM

FWIW, please note that this space-before-brace style is not specific to WebKit; CppCoreGuidelines exhibits it as well:
http://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#es23-prefer-the--initializer-syntax

This and WebKit's style seem like compelling arguments to support this option.

klimek, djasper: Do you have any objections against landing this?

Agreed.
Generally LG minus that I'd significantly reduce the number of test cases :)

unittests/Format/FormatTest.cpp
6867

There are super many redundant test cases here - I don't think we need to test that brace init detection works here, again.
I think given the code change we basically need 2 tests:
one where the previous opens a scope, and one where it doesn't.

Addressed feedback—thank you for the review!

rkirsling marked an inline comment as done.Jun 12 2018, 11:21 AM
hans added a comment.Jun 13 2018, 1:06 AM

Ross, do you have commit access or do you need someone to commit this for you?

I'll need someone to commit. Thanks!

This revision was automatically updated to reflect the committed changes.