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
_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
