[PATCH] D52676: [clang-format] tweaked another case of lambda formatting

[Oleg Smolsky via Phabricator via cfe-commits]

oleg.smolsky added a comment.

In https://reviews.llvm.org/D52676#1250124, @oleg.smolsky wrote:

> In https://reviews.llvm.org/D52676#1250071, @krasimir wrote:
>
> > In https://reviews.llvm.org/D52676#1249828, @oleg.smolsky wrote:
> >
> > > In https://reviews.llvm.org/D52676#1249784, @krasimir wrote:
> > >
> > > > IMO `BinPackArguments==false` does not imply that there should be a line break before the first arguments, only that there should not be two arguments from the same argument list that appear on the same line.
> > >
> > >
> > > That's right. However consider the following points:
> > >
> > > 1. a lambda function is already placed onto its own line when it is the first arg (as you can see in the first test)
> >
> >
> > I believe that a newline before a first arg lambda is controlled by a different mechanism independent of bin packing, probably by a rule that is more general than just "newline before first arg lambda". I think djasper@ could know more about this case.
>
>
> Hmm... perhaps. I have not been able to find an explicit rule for that.
>
> >> 1. the other args are placed onto a distinct line
> >> 2. this behavior looks very close to "bin packing"
> >> 3. adding a new option for the minor cases seems to be an overkill
> >> 
> >>   Having said that, I can add a new explicit style option. Do you think that will improve consensus? Would you expect test cases for positive and negative values of the option?
> > 
> > I don't see how the example before:
> > 
> >   void f() {
> >     something->Method2s(1,
> >                         [this] {
> >                           Do1();
> >                           Do2();
> >                         },
> >                         1);
> >   }
> > 
> > 
> > is inconsistent, as not adding a newline before the first argument is typical, as in:
> > 
> >   $ clang-format -style='{BasedOnStyle: google, BinPackArguments: false}' test.cc                                                             ~
> >   void f() {
> >     something->Method2s("111111111111111111",
> >                         "2222222222222222222222222222222222222222222222222222",
> >                         3);
> >   }
>
> Right, it's consistent with your example but inconsistent with with the following two:
>
> lambda at arg0:
>
>   void f() {
>     something->Method2s(
>         [this] {
>           Do1();
>           Do2();
>         },
>         1);
>   }
>
>
> and two lambdas:
>
>   // Multiple lambdas in the same parentheses change indentation rules.
>   verifyFormat("SomeFunction(\n"
>                "    []() {\n"
>                "      int i = 42;\n"
>                "      return i;\n"
>                "    },\n"
>                "    []() {\n"
>                "      int j = 43;\n"
>                "      return j;\n"
>                "    });");
>   




In https://reviews.llvm.org/D52676#1251064, @krasimir wrote:

> In https://reviews.llvm.org/D52676#1250124, @oleg.smolsky wrote:
>
> > In https://reviews.llvm.org/D52676#1250071, @krasimir wrote:
> >
> > > In https://reviews.llvm.org/D52676#1249828, @oleg.smolsky wrote:
> > >
> > > > In https://reviews.llvm.org/D52676#1249784, @krasimir wrote:
> > > >
> > > > > IMO `BinPackArguments==false` does not imply that there should be a line break before the first arguments, only that there should not be two arguments from the same argument list that appear on the same line.
> > > >
> > > >
> > > > That's right. However consider the following points:
> > > >
> > > > 1. a lambda function is already placed onto its own line when it is the first arg (as you can see in the first test)
> > >
> > >
> > > I believe that a newline before a first arg lambda is controlled by a different mechanism independent of bin packing, probably by a rule that is more general than just "newline before first arg lambda". I think djasper@ could know more about this case.
> >
> >
> > Hmm... perhaps. I have not been able to find an explicit rule for that.
> >
> > >> 1. the other args are placed onto a distinct line
> > >> 2. this behavior looks very close to "bin packing"
> > >> 3. adding a new option for the minor cases seems to be an overkill
> > >> 
> > >>   Having said that, I can add a new explicit style option. Do you think that will improve consensus? Would you expect test cases for positive and negative values of the option?
>
>
> Not really. It's generally hard to add a new style option, as that is mandated by an existing commonly used public style guide that requires it.
>
> >> is inconsistent, as not adding a newline before the first argument is typical, as in:
> >> 
> >>   $ clang-format -style='{BasedOnStyle: google, BinPackArguments: false}' test.cc                                                             ~
> >>   void f() {
> >>     something->Method2s("111111111111111111",
> >>                         "2222222222222222222222222222222222222222222222222222",
> >>                         3);
> >>   }
> > 
> > Right, it's consistent with your example but inconsistent with with the following two:
> > 
> > lambda at arg0:
> > 
> >   void f() {
> >     something->Method2s(
> >         [this] {
> >           Do1();
> >           Do2();
> >         },
> >         1);
> >   }
> > 
> > 
> > and two lambdas:
> > 
> >   // Multiple lambdas in the same parentheses change indentation rules.
> >   verifyFormat("SomeFunction(\n"
> >                "    []() {\n"
> >                "      int i = 42;\n"
> >                "      return i;\n"
> >                "    },\n"
> >                "    []() {\n"
> >                "      int j = 43;\n"
> >                "      return j;\n"
> >                "    });");
> >    
>
> The behavior you're observing here is a consequence of a more general rule: if the first argument is put on a new line, it's indented +4 spaces (I believe it's ContinuationIndentWidth) from the "current line"'s indent, for example also in:
>
>   int f() {
>      gggggggggggg(
>          "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa"
>          "bbb",
>          2);
>    }


Well, yes, it is. Yet lambdas have special treatment already (one lambda arg vs many, one statement inside it vs several). The existing layout bothers me because a single lambda arg gets indented too far to the right and, thus, forces excessive line breaks inside its block. Even more annoyingly, the indentation changes when it is the first arg, due to the extra line break. In that case the lambda's block has most of the code's width available (and no excessive line breaks).

So, the proposed change addresses some layout inconsistencies unconditionally and some are keyed off the "bin arg packing" option. And yes, that option is not ideal... but it sounds like adding a new option to "always add line breaks to long lambdas" would be even less appealing...

I believe that lambdas are formatted more consistently now.


Repository:
  rC Clang

https://reviews.llvm.org/D52676