russellmcc added a comment.
Thanks for the feedback! I'm very motivated to get some support for these
features since they are required for my style guide. Let me know if my recent
changes made sense to you.
================
Comment at: lib/Format/ContinuationIndenter.cpp:756
State.Line->MustBeDeclaration) ||
- Previous.is(TT_DictLiteral))
+ Previous.is(TT_DictLiteral) || !Style.AllowAllArgumentsOnNextLine)
State.Stack.back().BreakBeforeParameter = true;
----------------
djasper wrote:
> Hm, this looks suspicious. Doesn't this mean that AllowAllArgumentsOnNextLine
> implies/overwrites AllowAllParametersOfDeclarationOnNextLine?
>
> Now, there are two ways out of this:
> - Fix it
> - Provide different options
>
> The question is whether someone would ever want AllowAllArgumentsOnNextLine
> to be false while AllowAllParametersOfDeclarationOnNextLine is true. That
> would seem weird to me. If not, it might be better to turn the existing
> option into an enum with three values (None, Declarations, All) or something.
Oops! Thanks for finding this. I think since other options are exposed
separately for parameters and arguments (e.g., bin packing), it's more
consistent to break these out separately.
================
Comment at: lib/Format/ContinuationIndenter.cpp:962
State.Stack.back().NestedBlockIndent = State.Stack.back().Indent;
- if (Style.ConstructorInitializerAllOnOneLineOrOnePerLine)
+ if (Style.ConstructorInitializerAllOnOneLineOrOnePerLine) {
State.Stack.back().AvoidBinPacking = true;
----------------
djasper wrote:
> I am not 100%, but I think this is doing too much. In accordance with the
> other options, this should still allow:
>
> Constructor() : a(a), b(b) {}
>
> so long as everything fits on one line (and I think it might not). Either
> way, add a test.
I think the case you're talking about actually works; I've added a test.
================
Comment at: lib/Format/Format.cpp:748
} else {
+ ChromiumStyle.AllowAllArgumentsOnNextLine = true;
+ ChromiumStyle.AllowAllConstructorInitializersOnNextLine = true;
----------------
djasper wrote:
> I think there is no need to set these here and below. Everything derives from
> LLVM style.
Removed!
================
Comment at: unittests/Format/FormatTest.cpp:3440
+ Style.ColumnLimit = 60;
+ Style.ConstructorInitializerAllOnOneLineOrOnePerLine = true;
+ Style.AllowAllConstructorInitializersOnNextLine = true;
----------------
djasper wrote:
> If we go forward with this, the name of this option gets really confusing and
> we need to think about renaming it. Or maybe we can also turn it into an enum
> with three values?
>
> Here also, the combination of
> ConstructorInitializerAllOnOneLineOrOnePerLine=false and
> AllowAllConstructorInitializersOnNextLine=true
> seems really weird. I wouldn't even know what the desired behavior is in that
> case.
I agree this combination is weird, however the situation already existed with
declaration parameters (i.e., `AllowAllParametersOfDeclarationOnNextLine` has
no effect when `BinPackParameters` was true). I think exposing these new
parameters in this way is the most consistent with the existing approach.
I've edited the doc comment for `AllowAllConstructorInitializersOnNextLine ` to
note that it has no effect when
`ConstructorInitializerAllOnOneLineOrOnePerLine` is false.
https://reviews.llvm.org/D40988
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
