Page MenuHomePhabricator

[clang-format] tweaked another case of lambda formatting
ClosedPublic

Authored by oleg.smolsky on Sep 28 2018, 4:10 PM.

Details

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

Diff Detail

Repository
rL LLVM

Event Timeline

oleg.smolsky created this revision.Sep 28 2018, 4:10 PM
owenpan added a subscriber: owenpan.

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.

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 retitled this revision from lib/Format: tweaked another case of lambda formatting to [clang-format] tweaked another case of lambda formatting.Sep 29 2018, 3:48 AM
JonasToth edited the summary of this revision. (Show Details)
JonasToth set the repository for this revision to rC Clang.
JonasToth added a subscriber: JonasToth.EditedSep 29 2018, 3:53 AM

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;

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 edited the summary of this revision. (Show Details)Sep 29 2018, 9:14 AM
oleg.smolsky marked an inline comment as done.

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

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

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

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

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.

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.

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?

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.

@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!

oleg.smolsky edited the summary of this revision. (Show Details)Oct 10 2018, 10:38 AM

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 edited the summary of this revision. (Show Details)Oct 18 2018, 3:51 PM

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.

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.

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.

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

krasimir added inline comments.Oct 22 2018, 2:52 PM
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 edited the summary of this revision. (Show Details)

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 added inline comments.Oct 23 2018, 7:53 AM
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 marked 2 inline comments as done.Oct 23 2018, 10:58 AM
oleg.smolsky added inline comments.
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 marked 2 inline comments as done.Oct 23 2018, 3:55 PM
oleg.smolsky added inline comments.
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...

Corrected test regressions.

oleg.smolsky added inline comments.Oct 23 2018, 4:03 PM
clang/unittests/Format/FormatTest.cpp
78 ↗(On Diff #170773)

Oops., let me fix this...

Corrected test regressions, removed temporary hacks.

krasimir added inline comments.Oct 24 2018, 7:42 AM
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 added inline comments.Oct 24 2018, 9:29 AM
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");

Added a FIXME comment at Krasimir's request.

Folks, are there any other comments/suggestions?

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 marked 2 inline comments as done.Oct 31 2018, 8:38 AM
oleg.smolsky added inline comments.
unittests/Format/FormatTest.cpp
11854 ↗(On Diff #170915)

OK, sure, removed the scoped block.

11865 ↗(On Diff #170915)

Oh, right, that's vestigial. Removed.

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

oleg.smolsky marked 2 inline comments as done.

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 accepted this revision.Oct 31 2018, 10:58 AM

Thank you!

This revision is now accepted and ready to land.Oct 31 2018, 10:58 AM
This revision was automatically updated to reflect the committed changes.