djasper added a comment.
Generally, upload patches with the full file as diff context. Phabricator hides
that by default, but enables me to expand beyond the patch regions (where it
currently says "Context not available").
================
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;
----------------
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.
================
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;
----------------
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.
================
Comment at: lib/Format/Format.cpp:748
} else {
+ ChromiumStyle.AllowAllArgumentsOnNextLine = true;
+ ChromiumStyle.AllowAllConstructorInitializersOnNextLine = true;
----------------
I think there is no need to set these here and below. Everything derives from
LLVM style.
================
Comment at: unittests/Format/FormatTest.cpp:3440
+ Style.ColumnLimit = 60;
+ Style.ConstructorInitializerAllOnOneLineOrOnePerLine = true;
+ Style.AllowAllConstructorInitializersOnNextLine = true;
----------------
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.
================
Comment at: unittests/Format/FormatTest.cpp:3455
+ FormatStyle Style = getLLVMStyle();
+ Style.BreakConstructorInitializers = FormatStyle::BCIS_BeforeComma;
+ Style.ColumnLimit = 60;
----------------
Remove this line.
Repository:
rC Clang
https://reviews.llvm.org/D40988
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
