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