r263709 - clang-format: Slightly weaken AlignAfterOpenBracket=AlwaysBreak.
Daniel Jasper via cfe-commits
cfe-commits at lists.llvm.org
Thu Mar 24 13:20:18 PDT 2016
Hi Jean-Philippe,
first off, sorry that this is causing you trouble.
I am always happy to reconsider. However, I do think that this is a good
change based on hundreds of examples I have looked at. Obviously, this is
subjective and I won't even start to make an argument on whether this is
wasted or not.
We could add a separate option, but it is not quite as easy. AlwaysBreak
has never been *always* break. There have always been exceptions. Two
immediately spring to mind:
1. When the function name (plus open parenthesis) is shorter or equal to
the ContinuationIndentWidth.
2. For nested blocks such as lambdas, at least in some of the cases.
Both are also about wasting space. I think single argument function calls
are closely related to those two. So, we could add an "StrictAlwaysBreak"
mode and then also not have the two exemptions above, but I am not sure it
is worth it.
How often do these cases occur in your codebase? Do you really feel like
there is a significant readability disadvantage?
Cheers,
Daniel
On Sun, Mar 20, 2016 at 12:00 PM, Jean-philippe Dufraigne <
j.dufraigne at gmail.com> wrote:
> Hi Daniel,
>
> I'm not sure I understand the logic behind modifying 'AlwaysBreak' to be
> 'AlwaysBreakExceptForSingleArgument'.
>
> AlwaysBreak is always going to lead to "wasted space".
> Some consider consistently break to not be a waste but actually contribute
> to readability, it seems that was what 'AlwaysBreak' was about.
>
> Would you reconsider this change, or make it an option different from
> 'AlwaysBreak'?
>
>
>
> We just completed the roll out clang-format at my company.
> We are using: AlignAfterOpenBracket: AlwaysBreak
> We tuned the settings to have a style that does not clash too much with
> our original style, and that will look like a regression for us.
>
> Kind regards,
> Jean-Philippe.
>
>
> 2016-03-17 12:00 GMT+00:00 Daniel Jasper via cfe-commits <
> cfe-commits at lists.llvm.org>:
>
>> Author: djasper
>> Date: Thu Mar 17 07:00:22 2016
>> New Revision: 263709
>>
>> URL: http://llvm.org/viewvc/llvm-project?rev=263709&view=rev
>> Log:
>> clang-format: Slightly weaken AlignAfterOpenBracket=AlwaysBreak.
>>
>> If a call takes a single argument, using AlwaysBreak can lead to lots
>> of wasted lines and additional indentation without improving the
>> readability in a significant way.
>>
>> Before:
>> caaaaaaaaaaaall(
>> caaaaaaaaaaaall(
>> caaaaaaaaaaaall(
>> caaaaaaaaaaaaaaaaaaaaaaall(aaaaaaaaaaaaaa, aaaaaaaaa))));
>>
>> After:
>> caaaaaaaaaaaall(caaaaaaaaaaaall(caaaaaaaaaaaall(
>> caaaaaaaaaaaaaaaaaaaaaaall(aaaaaaaaaaaaaa, aaaaaaaaa))));
>>
>> Modified:
>> cfe/trunk/lib/Format/ContinuationIndenter.cpp
>> cfe/trunk/unittests/Format/FormatTest.cpp
>>
>> Modified: cfe/trunk/lib/Format/ContinuationIndenter.cpp
>> URL:
>> http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Format/ContinuationIndenter.cpp?rev=263709&r1=263708&r2=263709&view=diff
>>
>> ==============================================================================
>> --- cfe/trunk/lib/Format/ContinuationIndenter.cpp (original)
>> +++ cfe/trunk/lib/Format/ContinuationIndenter.cpp Thu Mar 17 07:00:22 2016
>> @@ -356,7 +356,17 @@ void ContinuationIndenter::addTokenOnCur
>> Previous.isOneOf(tok::l_paren, TT_TemplateOpener, tok::l_square) &&
>> State.Column > getNewLineColumn(State) &&
>> (!Previous.Previous ||
>> - !Previous.Previous->isOneOf(tok::kw_for, tok::kw_while,
>> tok::kw_switch)))
>> + !Previous.Previous->isOneOf(tok::kw_for, tok::kw_while,
>> + tok::kw_switch)) &&
>> + // Don't do this for simple (no expressions) one-argument function
>> calls
>> + // as that feels like needlessly wasting whitespace, e.g.:
>> + //
>> + // caaaaaaaaaaaall(
>> + // caaaaaaaaaaaall(
>> + // caaaaaaaaaaaall(
>> + // caaaaaaaaaaaaaaaaaaaaaaall(aaaaaaaaaaaaaa,
>> aaaaaaaaa))));
>> + Current.FakeLParens.size() > 0 &&
>> + Current.FakeLParens.back() > prec::Unknown)
>> State.Stack.back().NoLineBreak = true;
>>
>> if (Style.AlignAfterOpenBracket != FormatStyle::BAS_DontAlign &&
>>
>> Modified: cfe/trunk/unittests/Format/FormatTest.cpp
>> URL:
>> http://llvm.org/viewvc/llvm-project/cfe/trunk/unittests/Format/FormatTest.cpp?rev=263709&r1=263708&r2=263709&view=diff
>>
>> ==============================================================================
>> --- cfe/trunk/unittests/Format/FormatTest.cpp (original)
>> +++ cfe/trunk/unittests/Format/FormatTest.cpp Thu Mar 17 07:00:22 2016
>> @@ -4461,12 +4461,31 @@ TEST_F(FormatTest, AlignsAfterOpenBracke
>> " aaaaaaaaaaa aaaaaaaaa,\n"
>> " aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa);",
>> Style);
>> - verifyFormat("SomeLongVariableName->someFunction(\n"
>> - " foooooooo(\n"
>> - " aaaaaaaaaaaaaaa,\n"
>> - " aaaaaaaaaaaaaaaaaaaaa,\n"
>> - " aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa));",
>> + verifyFormat("SomeLongVariableName->someFunction(foooooooo(\n"
>> + " aaaaaaaaaaaaaaa,\n"
>> + " aaaaaaaaaaaaaaaaaaaaa,\n"
>> + " aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa));",
>> Style);
>> + verifyFormat(
>> + "aaaaaaaaaaaaaaaaaaaaaaaa(aaaaaaaaaaaaaaaaaaaaa(\n"
>> + " aaaaaaaaaaaaaaaaaaaa(aaaaaaaaaaaaaaaaa, aaaaaaaaaaaaaaaa)));",
>> + Style);
>> + verifyFormat(
>> + "aaaaaaaaaaaaaaaaaaaaaaaa(aaaaaaaaaa.aaaaaaaaaa(\n"
>> + " aaaaaaaaaaaaaaaaaaaa(aaaaaaaaaaaaaaaaa, aaaaaaaaaaaaaaaa)));",
>> + Style);
>> + verifyFormat(
>> + "aaaaaaaaaaaaaaaaaaaaaaaa(\n"
>> + " aaaaaaaaaaaaaaaaaaaaa(\n"
>> + " aaaaaaaaaaaaaaaaaaaa(aaaaaaaaaaaaaaaaa,
>> aaaaaaaaaaaaaaaa)),\n"
>> + " aaaaaaaaaaaaaaaa);",
>> + Style);
>> + verifyFormat(
>> + "aaaaaaaaaaaaaaaaaaaaaaaa(\n"
>> + " aaaaaaaaaaaaaaaaaaaaa(\n"
>> + " aaaaaaaaaaaaaaaaaaaa(aaaaaaaaaaaaaaaaa,
>> aaaaaaaaaaaaaaaa)) &&\n"
>> + " aaaaaaaaaaaaaaaa);",
>> + Style);
>> }
>>
>> TEST_F(FormatTest, ParenthesesAndOperandAlignment) {
>>
>>
>> _______________________________________________
>> cfe-commits mailing list
>> cfe-commits at lists.llvm.org
>> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
>>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20160324/c0a80ab9/attachment-0001.html>
More information about the cfe-commits
mailing list