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