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

[Oleg Smolsky via Phabricator via cfe-commits]

oleg.smolsky added a comment.

In https://reviews.llvm.org/D52676#1268806, @djasper wrote:

> In https://reviews.llvm.org/D52676#1268748, @oleg.smolsky wrote:
>
> > In https://reviews.llvm.org/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.


Repository:
  rC Clang

https://reviews.llvm.org/D52676