Page MenuHomePhabricator

Please use GitHub pull requests for new patches. Avoid migrating existing patches. Phabricator shutdown timeline

clang-format: Introduce BreakConstructorInitializers option

Authored by Typz on Apr 25 2017, 1:29 AM.



This option replaces the BreakConstructorInitializersBeforeComma option with an enum, thus introducing a mode where the colon stays on the same line as constructor declaration:

// When it fits on line:
Constructor() : initializer1(), initializer2() {}

// When it does not fit:
Constructor() :
    initializer1(), initializer2()

// When ConstructorInitializerAllOnOneLineOrOnePerLine = true:
Constructor() :

Diff Detail


Event Timeline

Typz created this revision.Apr 25 2017, 1:29 AM
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:, like Poco.
And I am indeed willing to maintain the patch.

Typz added a comment.May 16 2017, 7:03 AM

Or would it be better to replace (i.e. deprecate) the BreakConstructorInitializersBeforeComma option with a multiple choice option, as suggested here:

/// \brief Different ways to break initializers.
enum BreakConstructorInitializersStyle
  /// Constructor()
  ///     : initializer1(),
  ///       initializer2()
  /// Constructor()
  ///     : initializer1()
  ///     , initializer2()
  /// Constructor() :
  ///     initializer1(),
  ///     initializer2()
BreakConstructorInitializersStyle BreakConstructorInitializers
djasper edited edge metadata.May 17 2017, 5:57 AM

Yes, turning that option into an enum seems like the better choice here.

Typz updated this revision to Diff 99553.May 19 2017, 5:50 AM

Deprecate BreakConstructorInitializersBeforeComma and replace it with a more generic BreakConstructorInitializers option.

Typz retitled this revision from [clang-format] Add BreakConstructorInitializersBeforeColon option to clang-format: Introduce BreakConstructorInitializers option.May 19 2017, 6:16 AM
Typz edited the summary of this revision. (Show Details)
djasper added inline comments.May 22 2017, 1:44 AM
699 ↗(On Diff #99553)

I don't think you need to keep this around. The YAML parsing logic can correctly set the new field instead. So basically just call mapOptional for both names of configuration fields but always set BreakConstructorInitializers. And then map true/false to the appropriate enum values.

710 ↗(On Diff #99553)

Call this just "BeforeColon".

718 ↗(On Diff #99553)

Call this just "BeforeComma".

725 ↗(On Diff #99553)

Call this just "AfterColon".

61 ↗(On Diff #99553)

You don't need parentheses to surround comparisons. Remove them here and elsewhere.

196 ↗(On Diff #99553)

Why can you drop the "+2" here?

Also, I'd like to structure this so we have to duplicate less of the logic. But I am not really sure it's possible.

Typz updated this revision to Diff 99854.May 23 2017, 12:32 AM
Typz marked 5 inline comments as done.

respond to review comments

Typz added inline comments.May 23 2017, 12:32 AM
196 ↗(On Diff #99553)

the +2 here was needed to keep identifiers aligned when breaking after colon but before the command (e.g. the 4th combination, not defined anymore):

Foo() :
  , field(2)
196 ↗(On Diff #99553)

I can avoid some duplication like this,m but i am not convinced it helps :

const FormatToken &ColonToken =
    Style.BreakConstructorInitializers != FormatStyle::BCIS_AfterColon
                                          ? Current : Previous;
if ( &&
    (State.Column + State.Line->Last->TotalLength - ColonToken.TotalLength +
             (Style.BreakConstructorInitializers !=
                  FormatStyle::BCIS_AfterColon ? 2 : 0) >
         getColumnLimit(State) ||
     State.Stack.back().BreakBeforeParameter) &&
    (Style.AllowShortFunctionsOnASingleLine != FormatStyle::SFS_All ||
     Style.BreakConstructorInitializers != FormatStyle::BCIS_BeforeColon ||
     Style.ColumnLimit != 0))
  return true;

what do you think?

Typz updated this revision to Diff 99856.May 23 2017, 12:35 AM

Add missing doc for BreakConstructorInitializers

djasper added inline comments.May 23 2017, 4:34 AM
468 ↗(On Diff #99856)

Does this fit on one line now?

196 ↗(On Diff #99553)

The +2 here has nothing todo with how the things are aligned. This is about whether the entire constructor with initializer fits on one line. Can you try out (or even add tests) for cases where the entire constructor is 80 and 81 columns long?

I think I like the condensed version a bit better, but yeah, it's not beautiful either ;).

309 ↗(On Diff #99856)

How do you know that BreakConstructorInitializers was not specified?

Typz marked 6 inline comments as done.May 23 2017, 5:05 AM
Typz added inline comments.
468 ↗(On Diff #99856)

no, still 83 characters...

196 ↗(On Diff #99553)

My mistake, I read to quickly and talked about another +2 I removed from an earlier patch.

As far as I understand it, this +2 accounts for the the "upcoming" space and colon, when checking if breaking _before_ the colon (e.g. before it was added to the line).

Since this case is trying to break _after_ the colon, the space and colon have already been added to line (i.e. removed the column limit).

The tests are already included (and I have just double-checked: Constructor() : Initializer(FitsOnTheLine) {} is indeed 45 characters) :

verifyFormat("Constructor() : Initializer(FitsOnTheLine) {}",
             getStyleWithColumns(Style, 45));
verifyFormat("Constructor() :\n"
             "    Initializer(FitsOnTheLine) {}",
             getStyleWithColumns(Style, 44));
verifyFormat("Constructor() :\n"
             "    Initializer(FitsOnTheLine) {}",
             getStyleWithColumns(Style, 43));
309 ↗(On Diff #99856)

Technically, I don't ; but this is the default value, so it actually has the same effect...

Typz updated this revision to Diff 99886.May 23 2017, 5:06 AM
Typz marked 3 inline comments as done.

Refactor to avoid duplicating code, and fix typo in test.

djasper added inline comments.May 23 2017, 5:20 AM
196 ↗(On Diff #99553)

Ah, right. So as we are on the next token, State.Column will already include the +2. However, I think we should change that and make this always:

State.Column + State.Line->Last->TotalLength - Previous.TotalLength > getColumnLimit(State)

I think this should automatically add the "+2" or actually +1 should we go forward with your patch to not have a space before the colon at some point.

Typz updated this revision to Diff 99887.May 23 2017, 5:29 AM

Cleanup according to review comment

196 ↗(On Diff #99553)

Seems to work indeed, looking much better!
I had some trouble deciphering this when making my initial patch, and did not take the chance/risk to try to improve the 'regular' case.

djasper accepted this revision.May 24 2017, 3:21 AM

Looks good. Thank you!

196 ↗(On Diff #99553)

Yeah, not surprising. This code isn't exactly nice or easy to understand or well-commented :-(. Sorry about that.

This revision is now accepted and ready to land.May 24 2017, 3:21 AM
This revision was automatically updated to reflect the committed changes.