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

[Oleg Smolsky via Phabricator via cfe-commits]

oleg.smolsky marked 2 inline comments as done.
oleg.smolsky added inline comments.


================
Comment at: unittests/Format/FormatTest.cpp:11604
+               "      x.end(),   //\n"
+               "      [&](int, int) { return 1; });\n"
                "}\n");
----------------
djasper wrote:
> oleg.smolsky wrote:
> > krasimir wrote:
> > > 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.
> > 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)
> 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.
> 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.


================
Comment at: unittests/Format/FormatTest.cpp:11736
+  // line and there are no further args.
+  verifyFormat("function(1, [this, that] {\n"
+               "  //\n"
----------------
djasper wrote:
> oleg.smolsky wrote:
> > krasimir wrote:
> > > 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
> > > });
> > > ```
> > 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");
> > ```
> 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.
This behavior is consistent with 5.0 and 6.0, so we are OK.


Repository:
  rC Clang

https://reviews.llvm.org/D52676