[PATCH] D148131: Avoid unnecessarily aggressive line-breaking when using "LambdaBodyIndentation: OuterScope" with argument bin-packing.

Wed Apr 12 07:51:28 PDT 2023

jp4a50 added inline comments.

================
Comment at: clang/lib/Format/ContinuationIndenter.cpp:333
+      // enabled.
+      !(Current.is(TT_LambdaLBrace) && Style.BraceWrapping.BeforeLambdaBody) &&
       CurrentState.NoLineBreakInOperand) {
----------------
AFAICT, this only matters when using `OuterScope`, but it seems like a sensible exception to make in the general case, so I haven't included a check for `OuterScope` here.

================
Comment at: clang/lib/Format/ContinuationIndenter.cpp:1040
+      //
+      // We specifically special case "OuterScope"-formatted lambdas here
+      // because, when using that setting, breaking before the parameter
----------------
So I genuinely think that the original implementation here (which I've left intact *except* for when OuterScope is enabled) does not match up to the intention expressed in the comment in line 1018.

It effectively seems to assume that the parenthesis level entry which is second from the top is the 'parent' parenthesis level, but this ignores the fact that there are often "fake parens" entries in the parenthesis state stack which presumably should be ignored (as I have done in my implementation for OuterScope).

As such, there are a lot of cases where (when OuterScope is not enabled) we apply `BreakBeforeParameter` to the *existing* parenthesis level rather than the parent parenthesis level. So when I tried to "fix" this behaviour in the general case it broke a lot of tests, even though I think the behaviour may be what was originally intended.

Happy to discuss this and be proven wrong, but I thought I'd explain why I've added a special case here for OuterScope in even greater detail than I have in the comment.

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D148131/new/

https://reviews.llvm.org/D148131