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

[Francois Ferrand via Phabricator via cfe-commits]

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