[PATCH] D32479: clang-format: Introduce BreakConstructorInitializers option

[Daniel Jasper via Phabricator via cfe-commits]

djasper accepted this revision.
djasper added a comment.
This revision is now accepted and ready to land.

Looks good. Thank you!



================
Comment at: lib/Format/ContinuationIndenter.cpp:196
+           FormatStyle::BCIS_AfterColonAndComma) &&
+      (State.Column + State.Line->Last->TotalLength - Previous.TotalLength >
+           getColumnLimit(State) ||
----------------
Typz wrote:
> djasper wrote:
> > Typz wrote:
> > > djasper wrote:
> > > > Typz wrote:
> > > > > Typz wrote:
> > > > > > djasper wrote:
> > > > > > > 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.
> > > > > > 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(1)
> > > > > >     , field(2)
> > > > > 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 (ColonToken.is(TT_CtorInitializerColon) &&
> > > > >       (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?
> > > > 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 ;).
> > > 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));
> > 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.
> 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.
Yeah, not surprising. This code isn't exactly nice or easy to understand or well-commented :-(. Sorry about that.


https://reviews.llvm.org/D32479