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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
