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

[Daniel Jasper via Phabricator via cfe-commits]

djasper added inline comments.


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


================
Comment at: unittests/Format/FormatTest.cpp:11736
+  // line and there are no further args.
+  verifyFormat("function(1, [this, that] {\n"
+               "  //\n"
----------------
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.


Repository:
  rC Clang

https://reviews.llvm.org/D52676