[PATCH] D109557: Adds a BreakBeforeClosingParen option

[Cameron Mulhern via Phabricator via cfe-commits]

csmulhern added a comment.

In D109557#3063679 <https://reviews.llvm.org/D109557#3063679>, @MyDeveloperDay wrote:

> Do you think this is going to need some other capability to put the break after the  opening paren? e.g. `BreakAfterOpeningParen`
>
>   if (
>       ^

I don't think so. This is already implicitly dealt with based on the indentation of the line after the brace (which is affected by e.g. AlignAfterOpenBracket::BAS_AlwaysBreak).

In D109557#3063681 <https://reviews.llvm.org/D109557#3063681>, @MyDeveloperDay wrote:

> Personally I'm not convinced there wouldn't be people who want this for function declarations and definitions
>
>   Function(
>       param1,
>       param2,
>       param3
>   );
>
> but don't want this
>
>   if (
>      foo()
>   )
>   ....

To be clear, the if styling above would only occur if `foo()` was long enough to line wrap. Not all instances of `if`. However, I agree that could be true, and the existing clang-format code clearly treats indentation inside if / for / while differently than e.g. function calls. The existing BreakAfterOpeningParen options for example do not apply to the former for instance, which is why we see the weird indentation in the current revision where the opening brace is not followed by a line break, but the closing brace is preceded by one.

In D109557#3066186 <https://reviews.llvm.org/D109557#3066186>, @MyDeveloperDay wrote:

> Just as a general pattern what we see is that options start out as `bool`, shortly become `enums`, then as they get more complex become `structs` e.g. `BraceWrapping`
>
>   bool BreakBeforeClosingParen
>
> trying to think ahead a little can make future backwards compatibility a little easier.
>
> It will be a lot more involved but I kind of wonder if we might not see the same here over time. I could foresee a situation where we might have:
>
>   ParenWrapping:
>         StartOfIfOpening: true
>         EndOfIfExpression: true
>         FunctionParameterOpening: true
>         FunctionParameterClosing: true
>    
>
> of even a struct of enums (to allow special cases like short functions)
>
> I think if we could capture in unit tests the types of situation where we would and wouldn't want to put a newline after `(` and before `)` it might help define a better set of options in the first place.
>
> Otherwise if we are just going to use `BreakBeforeClosingParen` for all uses of `)` less the "short situations like c-style casts, then I kind of feel it should not impact parents around control statements like if,while,for etc... I think in which case I'd prefer we started out with a struct with just:  (ignore the actual names used, just something suitable)
>
>   ParenWrapping:
>         FunctionParametersClosing: <Never|Always|NonShortFunctions>

Yes, I completely agree. I had decided to propose we leave if / for / while outside the scope of BreakBeforeClosingParen for now, given that AlignAfterOpenBracket is also not applying to these situations. I've put together a revision that does this, but wanted to revisit the configuration option, because I can imagine wanting to extend this so that block indenting _does_ apply to if / for / while. The other revision that had attempted to do this work had landed on extending AlignAfterOpenBracket with a style AlwaysBreakAndCloseOnNextLine, which I think is appealing. I like the more explicit suggestion of ParenWrapping, but then I'm worried that you have these weird interactions between AlignAfterOpenBracket and ParenWrapping, and combinations that don't really make sense. What do you think of having something like BreakAfterOpeningParen::BlockIndent, which specifies the dangling parenthesis style? In the future, we can add a BlockIndentationStyle struct that can add finer control to block indentation, e.g. if block indentation is applied to if / for / while.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D109557