[clang-format] tweaked another case of lambda formatting

[oleg.smolsky]

Summary

This is done in order to improve cases where the lambda's body is moved too far to the right. Consider the following snippet with column limit set to 79:

void f() {
  leader::MakeThisCallHere(&leader_service_,
                           cq_.get(),
                           [this, liveness](const leader::ReadRecordReq& req,
                                            std::function<void()> done) {
                             logger_->HandleReadRecord(
                                 req, resp, std::move(done));
                           });

  leader::MakeAnother(&leader_service_,
                      cq_.get(),
                      [this, liveness](const leader::ReadRecordReq& req,
                                       std::function<void()> done) {
                        logger_->HandleReadRecord(
                            req, resp, std::move(done), a);
                      });
}

The tool favors extra indentation for the lambda body and so the code incurs extra wrapping and adjacent calls are indented to a different level. I find this behavior annoying and I'd like the tool to favor new lines and, thus, use the extra width.

The fix, reduced, brings the following formatting.

Before:

function(1,
         [] {
           DoStuff();
           //
         },
         1);

After:

function(
    1,
    [] {
      DoStuff();
      //
    },
    1);

Refer to the new tests in FormatTest.cpp

[krasimir]

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.

[oleg.smolsky]

In 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)
2. the other args are placed onto a distinct line
3. this behavior looks very close to "bin packing"
4. 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?

[JonasToth]

Please upload the patch with full context (i belive diff *edit* -U99999)

lib/Format/TokenAnnotator.cpp

3054 ↗

(On Diff #167567)

please no else after return. You can safely use

if (Left....)
   return true;
if (!Style...)
   return true;

[krasimir]

In D52676#1249828, @oleg.smolsky wrote:

In 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.

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);
}

[oleg.smolsky]

Tweaked if/else/return structure, added comments. No functional changes.

[oleg.smolsky]

In D52676#1250071, @krasimir wrote:

In D52676#1249828, @oleg.smolsky wrote:

In 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"
             "    });");

[krasimir]

In D52676#1250124, @oleg.smolsky wrote:

In D52676#1250071, @krasimir wrote:

In D52676#1249828, @oleg.smolsky wrote:

In 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);
}

[oleg.smolsky]

In D52676#1250124, @oleg.smolsky wrote:

In D52676#1250071, @krasimir wrote:

In D52676#1249828, @oleg.smolsky wrote:

In 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 D52676#1251064, @krasimir wrote:

In D52676#1250124, @oleg.smolsky wrote:

In D52676#1250071, @krasimir wrote:

In D52676#1249828, @oleg.smolsky wrote:

In 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.

[krasimir]

Digging a bit further, seems like the behavior you're looking for could be achieved by setting the AlignAfterOpenBracket option to DontAlign or AlwaysBreak:

% clang-format -style='{BasedOnStyle: google, AlignAfterOpenBracket: AlwaysBreak}' test.cc
void f() {
  something->Method2s(
      1,
      [this] {
        Do1();
        Do2();
      },
      1);
}
% clang-format -style='{BasedOnStyle: google, AlignAfterOpenBracket: DontAlign}' test.cc
void f() {
  something->Method2s(1,
      [this] {
        Do1();
        Do2();
      },
      1);
}

Does this work for you?

I don't think modifying the behavior as posed in this change based on the existence of multiline lambdas and the value of the BinPackArguments option is better than what we have now, especially when AlignAfterOpenBracket=Align.

[oleg.smolsky]

In D52676#1251342, @krasimir wrote:

Digging a bit further, seems like the behavior you're looking for could be achieved by setting the AlignAfterOpenBracket option to DontAlign or AlwaysBreak:

% clang-format -style='{BasedOnStyle: google, AlignAfterOpenBracket: AlwaysBreak}' test.cc
void f() {
  something->Method2s(
      1,
      [this] {
        Do1();
        Do2();
      },
      1);
}
% clang-format -style='{BasedOnStyle: google, AlignAfterOpenBracket: DontAlign}' test.cc
void f() {
  something->Method2s(1,
      [this] {
        Do1();
        Do2();
      },
      1);
}

Does this work for you?

This is interesting. It obviously does what I want with the lambda, but forces this:

void f() {
  auto stub = PopulateContextHereAndHereSomethi(GetSomethingHere(),
                                                GetSomethingElseHere());
}

into this:

void f() {
  auto stub = PopulateContextHereAndHereSomethi(
      GetSomethingHere(), GetSomethingElseHere());
}

The former looks better to me and that's what Emacs does when you press Tab. I think people here at work will balk at this formatting...

I don't think modifying the behavior as posed in this change based on the existence of multiline lambdas and the value of the BinPackArguments option is better than what we have now, especially when AlignAfterOpenBracket=Align.

Yeah, I hear you. The patch is intended to work with AlignAfterOpenBracket=Align and BinPackArguments=false and only tweaks the lambdas' placement.

How about I key the new behavior off a new value of AlignAfterOpenBracket, such as AlignAfterOpenBracket=AlignExceptLambda?

[krasimir]

In D52676#1251391, @oleg.smolsky wrote:

In D52676#1251342, @krasimir wrote:

Digging a bit further, seems like the behavior you're looking for could be achieved by setting the AlignAfterOpenBracket option to DontAlign or AlwaysBreak:

% clang-format -style='{BasedOnStyle: google, AlignAfterOpenBracket: AlwaysBreak}' test.cc
void f() {
  something->Method2s(
      1,
      [this] {
        Do1();
        Do2();
      },
      1);
}
% clang-format -style='{BasedOnStyle: google, AlignAfterOpenBracket: DontAlign}' test.cc
void f() {
  something->Method2s(1,
      [this] {
        Do1();
        Do2();
      },
      1);
}

Does this work for you?

This is interesting. It obviously does what I want with the lambda, but forces this:

void f() {
  auto stub = PopulateContextHereAndHereSomethi(GetSomethingHere(),
                                                GetSomethingElseHere());
}

into this:

void f() {
  auto stub = PopulateContextHereAndHereSomethi(
      GetSomethingHere(), GetSomethingElseHere());
}

The former looks better to me and that's what Emacs does when you press Tab. I think people here at work will balk at this formatting...

I don't think modifying the behavior as posed in this change based on the existence of multiline lambdas and the value of the BinPackArguments option is better than what we have now, especially when AlignAfterOpenBracket=Align.

Yeah, I hear you. The patch is intended to work with AlignAfterOpenBracket=Align and BinPackArguments=false and only tweaks the lambdas' placement.

How about I key the new behavior off a new value of AlignAfterOpenBracket, such as AlignAfterOpenBracket=AlignExceptLambda?

Sorry, I don't think we want to support something this specific. I'd like to hear djasper@ and klimek@ opinions about this.

[oleg.smolsky]

@djasper @klimek could you chime in please? This patch strives to cleanup a quirk in lambda formatting which pushes code too far to the right. Here is the problematic case:

void f() {
  something.something.something.Method(some_arg,
                                       [] {
                                           // the code here incurs
                                           // excessive wrapping
                                           // such as
                                           Method(
                                               some_med_arg,
                                               some_med_arg);
                                           some_var =
                                               some_expr + something;
                                      });

Here everything is technically correct, yet the code looks bad due to excessive wrapping. Things look a lot better when the lambda body starts from a new line, which happens already in some cases. The patch this "from the next line" onto a few more cases.

The full discussion of the cases with @krasimir is above.

Thanks!

[djasper]

Ok, I think I agree with both of you to a certain extent, but I also think this change as is, is not yet right.

First, it does too much. The original very first example in this CL is actually not produced by clang-format (or if it is, I don't know with which flag combination). It is a case where the lambda is the last parameter. For me, it actually produces:

void f() {
  something.something.something.Method(some_arg, [] {
    // the code here incurs
    // excessive wrapping
    // such as
    Method(some_med_arg, some_med_arg);
    some_var = some_expr + something;
  });
}

And that's an intentional optimization for a very common lambda use case. It reduces indentation even further and makes some coding patterns much nicer. I think (but haven't reproduced) that you patch might change the behavior there.

Second, I agree that the original behavior is inconsistent in a way that we have a special cases for when the lambda is the very first parameter, but somehow seem be forgetting about that when it's not the first parameter. I'd be ok with "fixing" that (it's not a clear-cut bug, but I think the alternative behavior would be cleaner). However, I don't think your patch is doing enough there. I think this should be irrespective of bin-packing (it's related but not quite the same thing), and it should also apply to multiline strings if AlwaysBreakBeforeMultilineStrings is true. It doesn't all have to be done in the same patch, but we should have a plan of what we want the end result to look like and should be willing to get there.

Maybe to start with, we need the complete test matrix so that we are definitely on the same page as to what we are trying to do. I imagine, we would need tests for a function with three parameters, where two of the parameters are short and one is a multiline lambda or a multiline string (and have the lambda be the first, second and third parameter). Then we might need those for both bin-packing and no-bin-packing configurations.

[oleg.smolsky]

In D52676#1268706, @djasper wrote:

Ok, I think I agree with both of you to a certain extent, but I also think this change as is, is not yet right.

First, it does too much. The original very first example in this CL is actually not produced by clang-format (or if it is, I don't know with which flag combination). It is a case where the lambda is the last parameter.

Right, I cheated and created that example by hand. My apologies for the confusion. I've just pasted real code that I pumped through clang-format. Please take a look at the updated summary.

Second, I agree that the original behavior is inconsistent in a way that we have a special cases for when the lambda is the very first parameter, but somehow seem be forgetting about that when it's not the first parameter. I'd be ok with "fixing" that (it's not a clear-cut bug, but I think the alternative behavior would be cleaner). However, I don't think your patch is doing enough there. I think this should be irrespective of bin-packing (it's related but not quite the same thing),

Also there is a special case for multiple lambdas. It forces line breaks. That aside, for the single-lambda case, are you suggesting that it is always "pulled up", irrespective of its place? That would contradict the direction I am trying to take as I like BinPackArguments: false and so long lamba args go to their own lines. This looks very much in line with what bin packing is, but not exactly the same. Obviously, I can add a new option favor line breaks around multi-line lambda.

Could you look at the updated summary (high level) and the new tests I added (low level) please? Every other test passes, so we have almost the entire spec. I can add a few cases where an existing snippet is reformatted with BinPackArguments: false too.

...and it should also apply to multiline strings if AlwaysBreakBeforeMultilineStrings is true. It doesn't all have to be done in the same patch, but we should have a plan of what we want the end result to look like and should be willing to get there.

Maybe to start with, we need the complete test matrix so that we are definitely on the same page as to what we are trying to do. I imagine, we would need tests for a function with three parameters, where two of the parameters are short and one is a multiline lambda or a multiline string (and have the lambda be the first, second and third parameter). Then we might need those for both bin-packing and no-bin-packing configurations.

[djasper]

In D52676#1268748, @oleg.smolsky wrote:

In D52676#1268706, @djasper wrote:

Ok, I think I agree with both of you to a certain extent, but I also think this change as is, is not yet right.

First, it does too much. The original very first example in this CL is actually not produced by clang-format (or if it is, I don't know with which flag combination). It is a case where the lambda is the last parameter.

Right, I cheated and created that example by hand. My apologies for the confusion. I've just pasted real code that I pumped through clang-format. Please take a look at the updated summary.

Second, I agree that the original behavior is inconsistent in a way that we have a special cases for when the lambda is the very first parameter, but somehow seem be forgetting about that when it's not the first parameter. I'd be ok with "fixing" that (it's not a clear-cut bug, but I think the alternative behavior would be cleaner). However, I don't think your patch is doing enough there. I think this should be irrespective of bin-packing (it's related but not quite the same thing),

Also there is a special case for multiple lambdas. It forces line breaks. That aside, for the single-lambda case, are you suggesting that it is always "pulled up", irrespective of its place? That would contradict the direction I am trying to take as I like BinPackArguments: false and so long lamba args go to their own lines. This looks very much in line with what bin packing is, but not exactly the same. Obviously, I can add a new option favor line breaks around multi-line lambda.

I don't think I am. You are right, there is the special case for multi-lambda functions and I think we should have almost the same for single-lambda functions. So, I think I agree with you and am in favor of:

someFunction(
    a,
    [] {
      // ...
    },
    b);

And this is irrespective of BinPacking. I think this is always better and more consistent with what we'd be doing if "a" was not there. The only caveat is that I think with BinPacking true or false, we should keep the more compact formatting if "b" isn't there and the lambda introducer fits entirely on the first line:

someFunction(a, [] {
  // ...
});

Could you look at the updated summary (high level) and the new tests I added (low level) please? Every other test passes, so we have almost the entire spec. I can add a few cases where an existing snippet is reformatted with BinPackArguments: false too.

Not sure I am seeing new test cases and I think at least a few cases are missing from the entire spec, e.g. the case above.

Also, try to reduce the test cases a bit more:

• They don't need the surrounding functions
• You can force the lambda to be multi-line with a "//" comment
• There is no reason to have the lambda be an argument to a member function, a free-standing function works just as well

This might seem nit-picky, but in my experience, the more we can reduce the test cases, the easier to read and the less brittle they become.

...and it should also apply to multiline strings if AlwaysBreakBeforeMultilineStrings is true. It doesn't all have to be done in the same patch, but we should have a plan of what we want the end result to look like and should be willing to get there.

Maybe to start with, we need the complete test matrix so that we are definitely on the same page as to what we are trying to do. I imagine, we would need tests for a function with three parameters, where two of the parameters are short and one is a multiline lambda or a multiline string (and have the lambda be the first, second and third parameter). Then we might need those for both bin-packing and no-bin-packing configurations.

[oleg.smolsky]

In D52676#1268806, @djasper wrote:

In D52676#1268748, @oleg.smolsky wrote:

In D52676#1268706, @djasper wrote:

Ok, I think I agree with both of you to a certain extent, but I also think this change as is, is not yet right.

First, it does too much. The original very first example in this CL is actually not produced by clang-format (or if it is, I don't know with which flag combination). It is a case where the lambda is the last parameter.

Right, I cheated and created that example by hand. My apologies for the confusion. I've just pasted real code that I pumped through clang-format. Please take a look at the updated summary.

Second, I agree that the original behavior is inconsistent in a way that we have a special cases for when the lambda is the very first parameter, but somehow seem be forgetting about that when it's not the first parameter. I'd be ok with "fixing" that (it's not a clear-cut bug, but I think the alternative behavior would be cleaner). However, I don't think your patch is doing enough there. I think this should be irrespective of bin-packing (it's related but not quite the same thing),

Also there is a special case for multiple lambdas. It forces line breaks. That aside, for the single-lambda case, are you suggesting that it is always "pulled up", irrespective of its place? That would contradict the direction I am trying to take as I like BinPackArguments: false and so long lamba args go to their own lines. This looks very much in line with what bin packing is, but not exactly the same. Obviously, I can add a new option favor line breaks around multi-line lambda.

I don't think I am. You are right, there is the special case for multi-lambda functions and I think we should have almost the same for single-lambda functions. So, I think I agree with you and am in favor of:

someFunction(
    a,
    [] {
      // ...
    },
    b);

And this is irrespective of BinPacking. I think this is always better and more consistent with what we'd be doing if "a" was not there. The only caveat is that I think with BinPacking true or false, we should keep the more compact formatting if "b" isn't there and the lambda introducer fits entirely on the first line:

someFunction(a, [] {
  // ...
});

OK, cool, I've generalized the patch and extended the test. This global thing also effects other existing test that deal with sub-blocks/lambdas.

Could you look at the updated summary (high level) and the new tests I added (low level) please? Every other test passes, so we have almost the entire spec. I can add a few cases where an existing snippet is reformatted with BinPackArguments: false too.

Not sure I am seeing new test cases and I think at least a few cases are missing from the entire spec, e.g. the case above.

Also, try to reduce the test cases a bit more:

• They don't need the surrounding functions
• You can force the lambda to be multi-line with a "//" comment
• There is no reason to have the lambda be an argument to a member function, a free-standing function works just as well

This might seem nit-picky, but in my experience, the more we can reduce the test cases, the easier to read and the less brittle they become.

Makes sense, reduced the extra levels.

...and it should also apply to multiline strings if AlwaysBreakBeforeMultilineStrings is true. It doesn't all have to be done in the same patch, but we should have a plan of what we want the end result to look like and should be willing to get there.

Oh, yeah, I will look at that next.

[oleg.smolsky]

Generalized the patch so that it deals with lambda args irrespective of bin packing. Added additional tests and patched existing ones.

[krasimir]

unittests/Format/FormatTest.cpp

11604 ↗

(On Diff #170270)

This looks a bit suspicious: I'd expect a break before the first arg to be forced only when there exists a multiline (after formatting) lambda expression arg. Is this (multiline vs. lambdas fitting 1 line) something that we (can) differentiate with respect to? djasper@ might have an insight on this.

11657 ↗

(On Diff #170270)

Similar to my previous comment, this forcing the std::function on a newline here might not be what we want to end up with?

11736 ↗

(On Diff #170270)

Could we please have a test case where there are several args packed on the first line, then a line break, then an arg, then a multiline lambda as a last arg (illustrating that we don't pull the first arg down if there's only a multiline lambda as the last arg):

function(a, b, ccccccc,
         d, [] () {
  body
});

[oleg.smolsky]

Added another test case.

unittests/Format/FormatTest.cpp

11604 ↗

(On Diff #170270)

Well, yes, I can see where you are coming from - the lambda is short and would fit. Unfortunately, I am not sure how to implement this nuance... I think I'd need to get the length of the unwrapped line and then check whether it fits in TokenUnnotator.cc....

Also, I personally favor less indentation (i.e. full width for the lambda) as that prevents drastic reformat when the lambda body changes. (that's why this patch exists)

11657 ↗

(On Diff #170270)

I think this change is in line with the updated/extended semantics - the extra arg forces the lambda to its own line and the introducer is kept with the preceding tokens.

11736 ↗

(On Diff #170270)

Sure, that seems to work, but not in the way you expected :) I'll update the patch...

verifyFormat("function(a, b, c, //\n"
             "         d, [this, that] {\n"
             "           //\n"
             "         });\n");

[djasper]

unittests/Format/FormatTest.cpp

11604 ↗

(On Diff #170270)

I agree with Krasimir here.

If you prefer less indentation, great. Set AlignAfterOpenBracket to "AlwaysBreak" and we don't need any of this ;-) (as Krasimir has suggested before).

In more seriousness, I think getting all these cases right, I appreciate that it is difficult. However, I also like to make sure that we do get them right. Changing clang-format's behavior for any of these cases is not a small thing, it will cause churn for *a lot* of people. We should try really hard to not have changes in there that people will find detrimental. This of course is subjective, so we won't get to 100%, but if in doubt for specific cases, let's err on the side of not changing the current behavior.

11736 ↗

(On Diff #170270)

We should try to prevent that (unless it's also the current behavior of course). People have filed various bugs about this before and it is not generally an accepted formatting.

[oleg.smolsky]

unittests/Format/FormatTest.cpp

11604 ↗

(On Diff #170270)

If you prefer less indentation, great. Set AlignAfterOpenBracket to "AlwaysBreak" and we don't need any of this ;-) (as Krasimir has suggested before).

Well, AlignAfterOpenBracket is too big a gun as we talking about lambda function args here.

In more seriousness, I think getting all these cases right, I appreciate that it is difficult. However, I also like to make sure that we do get them right. Changing clang-format's behavior for any of these cases is not a small thing, it will cause churn for *a lot* of people. We should try really hard to not have changes in there that people will find detrimental. This of course is subjective, so we won't get to 100%, but if in doubt for specific cases, let's err on the side of not changing the current behavior.

OK, that's fair. Let me try to figure out how to get the former behavior restored for this case.

11736 ↗

(On Diff #170270)

This behavior is consistent with 5.0 and 6.0, so we are OK.

[oleg.smolsky]

unittests/Format/FormatTest.cpp

11604 ↗

(On Diff #170270)

OK, I've just implemented heuristics for counting the introducer's length, but it turns out that it's not needed. I just had a bug in one of the conditions... So, I've added a couple more tests and will try posting a patch against the new monorepo momentarily...

[oleg.smolsky]

Corrected test regressions.

[oleg.smolsky]

clang/unittests/Format/FormatTest.cpp

78 ↗

(On Diff #170773)

Oops., let me fix this...

[oleg.smolsky]

Corrected test regressions, removed temporary hacks.

[krasimir]

unittests/Format/FormatTest.cpp

11736 ↗

(On Diff #170270)

Maybe add a FIXME for that test that this is not ideal formatting and we should also be pulling the first arg on a newline in that case too in the future then.

[oleg.smolsky]

unittests/Format/FormatTest.cpp

11736 ↗

(On Diff #170270)

Sure, added a comment:

// FIXME: this format is not ideal and we should consider forcing the first arg
// onto its own line.
verifyFormat("function(a, b, c, //\n"
             "         d, [this, that] {\n"
             "           //\n"
             "         });\n");

[oleg.smolsky]

Added a FIXME comment at Krasimir's request.

[oleg.smolsky]

Folks, are there any other comments/suggestions?

[djasper]

I think this roughly looks fine. Krasimir, any thoughts?

unittests/Format/FormatTest.cpp

11854 ↗

(On Diff #170915)

No need to use a scope here. Feel free to redefine Style.

If in fact you feel like that is getting out of hand, maybe extract a separate TEST_F() for this.

11865 ↗

(On Diff #170915)

No need for this scope.

[oleg.smolsky]

unittests/Format/FormatTest.cpp

11854 ↗

(On Diff #170915)

OK, sure, removed the scoped block.

11865 ↗

(On Diff #170915)

Oh, right, that's vestigial. Removed.

[krasimir]

Looks good! Will stamp when the scopes are removed. Oleg, do you need someone to commit this for you?

[oleg.smolsky]

In D52676#1282335, @krasimir wrote:

Looks good! Will stamp when the scopes are removed.

Cool, thanks, Krasimir. I've just posted the updated patch.

Oleg, do you need someone to commit this for you?

I do, could you commit this please?

[krasimir]

Thank you!

[jaafar]

Hi everyone, I've been investigating a bug and bisected it to this commit.

The problem, in brief, is that lambdas passed as arguments can cause an extra line break before the first argument, but only if the lambda is neither the first nor the last argument. This implies, of course, that there are at least three arguments supplied... Perhaps the most minimal example is:

void f()
  {
      func(0, [] {}, 0);
  }

which gets formatted as:

void
f ()
{
    func (
        0, [] {}, 0);
}

(note the unnecessary newline after the left paren)

I found three existing reports of this problem: 28546, 45141, and 45424. I'm posting here in lieu of creating a fourth duplicate bug report, in the hopes that it will be more likely to find someone who can fix it. Thank you.

[jaafar]

It's been pointed out to me that 28546 preceded this commit... so it may only appear related.

[jaafar]

cfe/trunk/lib/Format/TokenAnnotator.cpp

3072

This is the clause that triggers the problem.

[curdeius]

Hi,
I know it's an old revision, but I confirm that it provoked the bug https://bugs.llvm.org/show_bug.cgi?id=45141.
The problem is in the TokenAnnotator::mustBreakBefore as @jaafar pointed out:

if (!Next->isOneOf(TT_LambdaLSquare, tok::l_brace, tok::caret))
  return true; // << HERE

It should return false if everything fits on a single line.
I'm not sure how to check this though.
@MyDeveloperDay, would you have an idea?

[MyDeveloperDay]

I think if you have a proposal for changing the behavior lets see the patch and how it impacts the existing unit tests

something tells me these tests are going to change?

// A lambda passed as arg0 is always pushed to the next line.
verifyFormat("SomeFunction(\n"
             "    [this] {\n"
             "      //\n"
             "    },\n"
             "    1);\n");


// A multi-line lambda passed as arg1 forces arg0 to be pushed out, just like the arg0
// case above.
auto Style = getGoogleStyle();
Style.BinPackArguments = false;
verifyFormat("SomeFunction(\n"
             "    a,\n"
             "    [this] {\n"
             "      //\n"
             "    },\n"
             "    b);\n",
             Style);
verifyFormat("SomeFunction(\n"
             "    a,\n"
             "    [this] {\n"
             "      //\n"
             "    },\n"
             "    b);\n");

[oleg.smolsky]

@MyDeveloperDay that's the exact point. I authored this change to close a gap in some lambda formatting cases. The tests (existing, modified and added) express all relevant cases that I knew at the time.

@jaafar, @curdeius - this is just C++ code. Nothing is set in stone. Of course there are other cases that are effected by this change. The right way forward here is for you guys to author a new change. Ideally, the change will bring the new test, format it to your liking and change nothing else.

[MyDeveloperDay]

@oleg.smolsky I agree, what you have here covers a myriad of other cases and was committed in 2018, we can't call this commit a regression it was a feature ;-), if we want to improve the feature that is something else.

[curdeius]

I agree with both of you. I shouldn't have used the word "regression" indeed. I just meant a change in behaviour. Sorry for that.
I'll try to play around and propose a patch to enhance this feature :).
If you have any pointers about how to check if everything fits on a single line (it must be already done somewhere, nope?) then I'm all ears!

cfe/trunk/lib/Format/ContinuationIndenter.cpp

unsigned ContinuationIndenter::moveStateToNextToken(LineState &State,
   // Add special behavior to support a format commonly used for JavaScript
   // closures:
   //   SomeFunction(function() {
   //     foo();
   //     bar();
   //   }, a, b, c);
   if (Current.isNot(tok::comment) && Previous &&
       Previous->isOneOf(tok::l_brace, TT_ArrayInitializerLSquare) &&
-      !Previous->is(TT_DictLiteral) && State.Stack.size() > 1) {
+      !Previous->is(TT_DictLiteral) && State.Stack.size() > 1 &&
+      !State.Stack.back().HasMultipleNestedBlocks) {
     if (State.Stack[State.Stack.size() - 2].NestedBlockInlined && Newline)
       for (unsigned i = 0, e = State.Stack.size() - 1; i != e; ++i)
         State.Stack[i].NoLineBreak = true;
     State.Stack[State.Stack.size() - 2].NestedBlockInlined = false;
   }
   if (Previous &&
       (Previous->isOneOf(tok::l_paren, tok::comma, tok::colon) ||
        Previous->isOneOf(TT_BinaryOperator, TT_ConditionalExpr)) &&

cfe/trunk/lib/Format/FormatToken.h

virtual unsigned formatAfterToken(LineState &State,
                                     ContinuationIndenter *Indenter,
                                     bool DryRun) {
     return 0;
   }
 
   /// Notifies the \c Role that a comma was found.
   virtual void CommaFound(const FormatToken *Token) {}
 
+  virtual const FormatToken *lastComma() { return nullptr; }
+
 protected:
   const FormatStyle &Style;
 };
 
 class CommaSeparatedList : public TokenRole {
 public:
   CommaSeparatedList(const FormatStyle &Style)
       : TokenRole(Style), HasNestedBracedList(false) {}
 
   void precomputeFormattingInfos(const FormatToken *Token) override;
 
   unsigned formatAfterToken(LineState &State, ContinuationIndenter *Indenter,
                             bool DryRun) override;
 
   unsigned formatFromToken(LineState &State, ContinuationIndenter *Indenter,
                            bool DryRun) override;
 
   /// Adds \p Token as the next comma to the \c CommaSeparated list.
   void CommaFound(const FormatToken *Token) override {
     Commas.push_back(Token);
   }
 
+  const FormatToken *lastComma() override {
+    if (Commas.empty())
+      return nullptr;
+    return Commas.back();
+  }
+
 private:
   /// A struct that holds information on how to format a given list with
   /// a specific number of columns.
   struct ColumnFormat {
     /// The number of columns to use.
     unsigned Columns;
 
     /// The total width in characters.

cfe/trunk/lib/Format/TokenAnnotator.cpp

if ((Style.Language == FormatStyle::LK_Proto ||
     //            v  v
     // sub: { ... } key: value
     // If there was a comment between `}` an `key` above, then `key` would be
     // put on a new line anyways.
     if (Left.isOneOf(tok::r_brace, tok::greater, tok::r_square))
       return true;
   }
 
+  // Deal with lambda arguments in C++ - we want consistent line breaks whether
+  // they happen to be at arg0, arg1 or argN. The selection is a bit nuanced
+  // as aggressive line breaks are placed when the lambda is not the last arg.
+  if ((Style.Language == FormatStyle::LK_Cpp ||
+       Style.Language == FormatStyle::LK_ObjC) &&
+      Left.is(tok::l_paren) && Left.BlockParameterCount > 0 &&
+      !Right.isOneOf(tok::l_paren, TT_LambdaLSquare)) {
+    // Multiple lambdas in the same function call force line breaks.
+    if (Left.BlockParameterCount > 1)
+      return true;
+
+    // A lambda followed by another arg forces a line break.
+    if (!Left.Role)
+      return false;
+    auto Comma = Left.Role->lastComma();
+    if (!Comma)
+      return false;
+    auto Next = Comma->getNextNonComment();
+    if (!Next)
+      return false;
+    if (!Next->isOneOf(TT_LambdaLSquare, tok::l_brace, tok::caret))
+      return true;
+  }
+
   return false;
 }
 
 bool TokenAnnotator::canBreakBefore(const AnnotatedLine &Line,
                                     const FormatToken &Right) {
   const FormatToken &Left = *Right.Previous;
 
   // Language-specific stuff.

cfe/trunk/lib/Format/UnwrappedLineFormatter.cpp

private:
   /// solution space that leads to \c Best.
   void reconstructPath(LineState &State, StateNode *Best) {
     std::deque<StateNode *> Path;
     // We do not need a break before the initial token.
     while (Best->Previous) {
       Path.push_front(Best);
       Best = Best->Previous;
     }
-    for (std::deque<StateNode *>::iterator I = Path.begin(), E = Path.end();
-         I != E; ++I) {
+    for (auto I = Path.begin(), E = Path.end(); I != E; ++I) {
       unsigned Penalty = 0;
       formatChildren(State, (*I)->NewLine, /*DryRun=*/false, Penalty);
       Penalty += Indenter->addTokenToState(State, (*I)->NewLine, false);
 
       LLVM_DEBUG({
         printLineState((*I)->Previous->State);
         if ((*I)->NewLine) {
           llvm::dbgs() << "Penalty for placing "
-                       << (*I)->Previous->State.NextToken->Tok.getName() << ": "
-                       << Penalty << "\n";
+                       << (*I)->Previous->State.NextToken->Tok.getName()
+                       << " on a new line: " << Penalty << "\n";
         }
       });
     }
   }
 
   llvm::SpecificBumpPtrAllocator<StateNode> Allocator;
 };
 

cfe/trunk/unittests/Format/FormatTest.cpp

verifyFormat("DEBUG({\n"
                "  if (a)\n"
                "    return;\n"
                "});");
   verifyGoogleFormat("DEBUG({\n"
                      "  if (a) return;\n"
                      "});");
   FormatStyle Style = getGoogleStyle();
   Style.ColumnLimit = 45;
-  verifyFormat("Debug(aaaaa,\n"
-               "      {\n"
-               "        if (aaaaaaaaaaaaaaaaaaaaaaaa) return;\n"
-               "      },\n"
-               "      a);",
+  verifyFormat("Debug(\n"
+               "    aaaaa,\n"
+               "    {\n"
+               "      if (aaaaaaaaaaaaaaaaaaaaaaaa) return;\n"
+               "    },\n"
+               "    a);",
                Style);
 
   verifyFormat("SomeFunction({MACRO({ return output; }), b});");
 
   verifyNoCrash("^{v^{a}}");
 }
 
 TEST_F(FormatTest, FormatNestedBlocksInMacros) {
TEST_F(FormatTest, FormatsLambdas) {
   verifyFormat("void f() {\n"
                "  other(x.begin(), x.end(), [&](int, int) { return 1; });\n"
                "}\n");
   verifyFormat("void f() {\n"
                "  other(x.begin(), //\n"
                "        x.end(),   //\n"
                "        [&](int, int) { return 1; });\n"
                "}\n");
+  verifyFormat("void f() {\n"
+               "  other.other.other.other.other(\n"
+               "      x.begin(), x.end(),\n"
+               "      [something, rather](int, int, int, int, int, int, int) { return 1; });\n"
+               "}\n");
+  verifyFormat("void f() {\n"
+               "  other.other.other.other.other(\n"
+               "      x.begin(), x.end(),\n"
+               "      [something, rather](int, int, int, int, int, int, int) {\n"
+               "        //\n"
+               "      });\n"
+               "}\n");
   verifyFormat("SomeFunction([]() { // A cool function...\n"
                "  return 43;\n"
                "});");
   EXPECT_EQ("SomeFunction([]() {\n"
             "#define A a\n"
             "  return 43;\n"
             "});",
             format("SomeFunction([](){\n"
verifyFormat("SomeFunction({[&] {\n"
                "                // comment\n"
                "              },\n"
                "              [&] {\n"
                "                // comment\n"
                "              }});");
   verifyFormat("SomeFunction({[&] {\n"
                "  // comment\n"
                "}});");
-  verifyFormat("virtual aaaaaaaaaaaaaaaa(std::function<bool()> bbbbbbbbbbbb =\n"
-               "                             [&]() { return true; },\n"
-               "                         aaaaa aaaaaaaaa);");
+  verifyFormat("virtual aaaaaaaaaaaaaaaa(\n"
+               "    std::function<bool()> bbbbbbbbbbbb = [&]() { return true; },\n"
+               "    aaaaa aaaaaaaaa);");
 
   // Lambdas with return types.
   verifyFormat("int c = []() -> int { return 2; }();\n");
   verifyFormat("int c = []() -> int * { return 2; }();\n");
   verifyFormat("int c = []() -> vector<int> { return {2}; }();\n");
   verifyFormat("Foo([]() -> std::vector<int> { return {2}; }());");
   verifyGoogleFormat("auto a = [&b, c](D* d) -> D* {};");
   verifyGoogleFormat("auto a = [&b, c](D* d) -> pair<D*, D*> {};");
verifyFormat(
       "      return aaaaaaaaaaaaaaaaa;\n"
       "    });",
       getLLVMStyleWithColumns(70));
   verifyFormat("[]() //\n"
                "    -> int {\n"
                "  return 1; //\n"
                "};");
 
-  // Multiple lambdas in the same parentheses change indentation rules.
+  // Multiple lambdas in the same parentheses change indentation rules. These
+  // lambdas are forced to start on new lines.
   verifyFormat("SomeFunction(\n"
                "    []() {\n"
-               "      int i = 42;\n"
-               "      return i;\n"
+               "      //\n"
                "    },\n"
                "    []() {\n"
-               "      int j = 43;\n"
-               "      return j;\n"
+               "      //\n"
                "    });");
 
+  // A lambda passed as arg0 is always pushed to the next line.
+  verifyFormat("SomeFunction(\n"
+               "    [this] {\n"
+               "      //\n"
+               "    },\n"
+               "    1);\n");
+
+  // A multi-line lambda passed as arg1 forces arg0 to be pushed out, just like the arg0
+  // case above.
+  auto Style = getGoogleStyle();
+  Style.BinPackArguments = false;
+  verifyFormat("SomeFunction(\n"
+               "    a,\n"
+               "    [this] {\n"
+               "      //\n"
+               "    },\n"
+               "    b);\n",
+               Style);
+  verifyFormat("SomeFunction(\n"
+               "    a,\n"
+               "    [this] {\n"
+               "      //\n"
+               "    },\n"
+               "    b);\n");
+
+  // A lambda with a very long line forces arg0 to be pushed out irrespective of
+  // the BinPackArguments value (as long as the code is wide enough).
+  verifyFormat("something->SomeFunction(\n"
+               "    a,\n"
+               "    [this] {\n"
+               "      D0000000000000000000000000000000000000000000000000000000000001();\n"
+               "    },\n"
+               "    b);\n");
+
+  // A multi-line lambda is pulled up as long as the introducer fits on the previous
+  // line and there are no further args.
+  verifyFormat("function(1, [this, that] {\n"
+               "  //\n"
+               "});\n");
+  verifyFormat("function([this, that] {\n"
+               "  //\n"
+               "});\n");
+  // FIXME: this format is not ideal and we should consider forcing the first arg
+  // onto its own line.
+  verifyFormat("function(a, b, c, //\n"
+               "         d, [this, that] {\n"
+               "           //\n"
+               "         });\n");
+
+  // Multiple lambdas are treated correctly even when there is a short arg0.
+  verifyFormat("SomeFunction(\n"
+               "    1,\n"
+               "    [this] {\n"
+               "      //\n"
+               "    },\n"
+               "    [this] {\n"
+               "      //\n"
+               "    },\n"
+               "    1);\n");
+
   // More complex introducers.
   verifyFormat("return [i, args...] {};");
 
   // Not lambdas.
   verifyFormat("constexpr char hello[]{\"hello\"};");
   verifyFormat("double &operator[](int i) { return 0; }\n"
                "int i;");
   verifyFormat("std::unique_ptr<int[]> foo() {}");