djasper added a comment.
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, [] { // ... }); > 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. Repository: rC Clang https://reviews.llvm.org/D52676 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
