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

[Daniel Jasper via Phabricator via cfe-commits]

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:
> 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 ;).


================
Comment at: lib/Format/ContinuationIndenter.cpp:468
+  } else if (Previous.is(TT_CtorInitializerColon) &&
+             Style.BreakConstructorInitializers ==
+                 FormatStyle::BCIS_AfterColon) {
----------------
Does this fit on one line now?


================
Comment at: lib/Format/Format.cpp:309
+    // If BreakConstructorInitializersBeforeComma was specified but
+    // BreakConstructorInitializers was not, initialize the latter from the
+    // former for backwards compatibility.
----------------
How do you know that BreakConstructorInitializers was not specified?


https://reviews.llvm.org/D32479