djasper added inline comments.
================
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:
> > > 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.
https://reviews.llvm.org/D32479
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
