[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#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.




Repository:
  rC Clang

https://reviews.llvm.org/D52676