Typz marked 6 inline comments as done.
Typz added inline comments.
================
Comment at: lib/Format/ContinuationIndenter.cpp:196
+ FormatStyle::BCIS_AfterColonAndComma) &&
+ (State.Column + State.Line->Last->TotalLength - Previous.TotalLength >
+ getColumnLimit(State) ||
----------------
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));
================
Comment at: lib/Format/ContinuationIndenter.cpp:468
+ } else if (Previous.is(TT_CtorInitializerColon) &&
+ Style.BreakConstructorInitializers ==
+ FormatStyle::BCIS_AfterColon) {
----------------
djasper wrote:
> Does this fit on one line now?
no, still 83 characters...
================
Comment at: lib/Format/Format.cpp:309
+ // If BreakConstructorInitializersBeforeComma was specified but
+ // BreakConstructorInitializers was not, initialize the latter from the
+ // former for backwards compatibility.
----------------
djasper wrote:
> How do you know that BreakConstructorInitializers was not specified?
Technically, I don't ; but this is the default value, so it actually has the
same effect...
https://reviews.llvm.org/D32479
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
