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