r263709 - clang-format: Slightly weaken AlignAfterOpenBracket=AlwaysBreak.

[Jean-philippe Dufraigne via cfe-commits]

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/20160320/38ecf388/attachment.html>