Adds a BlockIndent option to AlignAfterOpenBracket

[csmulhern]

Summary

This style is similar to AlwaysBreak, but places closing brackets on new lines.

For example, if you have a multiline parameter list, clang-format currently only supports breaking per-parameter, but places the closing bracket on the line of the last parameter.

Function(
    param1,
    param2,
    param3);

A style supported by other code styling tools (e.g. rustfmt) is to allow the closing brackets to be placed on their own line, aiding the user in being able to quickly infer the bounds of the block of code.

Function(
    param1,
    param2,
    param3
);

For prior work on a similar feature, see: https://reviews.llvm.org/D33029.

Note: This currently only supports block indentation for closing parentheses.

[MyDeveloperDay]

Please resubmit the patch with full context

[csmulhern]

I've added more information to my original message. Please let me know if further context is desired.

[HazardyKnusperkeks]

In D109557#2997899, @csmulhern wrote:

I've added more information to my original message. Please let me know if further context is desired.

With context he meant the diff context. https://llvm.org/docs/Phabricator.html#requesting-a-review-via-the-web-interface

[HazardyKnusperkeks]

You state in the documentation that it is also for angle brackets and more, but there are no test cases for that.

[csmulhern]

Adds more context to the diff.

[csmulhern]

In D109557#2998213, @HazardyKnusperkeks wrote:

With context he meant the diff context. https://llvm.org/docs/Phabricator.html#requesting-a-review-via-the-web-interface

Ah sorry about that. Done.

In D109557#2998226, @HazardyKnusperkeks wrote:

You state in the documentation that it is also for angle brackets and more, but there are no test cases for that.

Yeah, I wasn't sure exactly how to deal with this. The default behavior is already to align angle brackets and braces on newlines. See: https://github.com/llvm/llvm-project/blob/8a780a2f18c590e27e51a2ab3cc81b481c42b42a/clang/lib/Format/ContinuationIndenter.cpp#L341 (BreakBeforeClosingBrace is true when the block was started with a newline). Thus, you're already getting this behavior when CBAS_AlwaysBreak is set. I didn't want to make DontAlign (the default) explicitly opt out of this behavior. I guess we can narrow the scope of CloseBracketAlignmentStyle to just parenthesis, but that doesn't feel great either. What are your thoughts?

[MyDeveloperDay]

This isn't really AlignCloseBracket but BreakBeforeClosingParen isn't it?

[HazardyKnusperkeks]

In D109557#2998727, @csmulhern wrote:

In D109557#2998213, @HazardyKnusperkeks wrote:

With context he meant the diff context. https://llvm.org/docs/Phabricator.html#requesting-a-review-via-the-web-interface

Ah sorry about that. Done.

No Problem.

In D109557#2998226, @HazardyKnusperkeks wrote:

You state in the documentation that it is also for angle brackets and more, but there are no test cases for that.

Yeah, I wasn't sure exactly how to deal with this. The default behavior is already to align angle brackets and braces on newlines. See: https://github.com/llvm/llvm-project/blob/8a780a2f18c590e27e51a2ab3cc81b481c42b42a/clang/lib/Format/ContinuationIndenter.cpp#L341 (BreakBeforeClosingBrace is true when the block was started with a newline). Thus, you're already getting this behavior when CBAS_AlwaysBreak is set. I didn't want to make DontAlign (the default) explicitly opt out of this behavior. I guess we can narrow the scope of CloseBracketAlignmentStyle to just parenthesis, but that doesn't feel great either. What are your thoughts?

I haven't looked too much into it, my main point is that there should be tests for both variants of that option for braces, parenthesis, and angular braces, if they are handled by that option. Otherwise the documentation (and naming?) should be adapted. If the defaults differ, the option has to be reworked, for a finer control.

In D109557#2999085, @MyDeveloperDay wrote:

This isn't really AlignCloseBracket but BreakBeforeClosingParen isn't it?

Yeah I thought that too.

[csmulhern]

In D109557#3000152, @HazardyKnusperkeks wrote:

I haven't looked too much into it, my main point is that there should be tests for both variants of that option for braces, parenthesis, and angular braces, if they are handled by that option. Otherwise the documentation (and naming?) should be adapted. If the defaults differ, the option has to be reworked, for a finer control.

I went ahead and limited this in scope to explicitly only deal with closing parentheses.

In D109557#2999085, @MyDeveloperDay wrote:

This isn't really AlignCloseBracket but BreakBeforeClosingParen isn't it?

Yeah I thought that too.

Good call. I've renamed the option to BreakBeforeClosingParen.

[HazardyKnusperkeks]

clang/unittests/Format/FormatTest.cpp

22301

Could you also add tests for false, even though they are spread over the other test cases. That way the option can completely be tested with this case.

[csmulhern]

clang/unittests/Format/FormatTest.cpp

22301

Done.

[HazardyKnusperkeks]

Looks good, but please wait for MyDeveloperDay’s opinion.

[MyDeveloperDay]

clang/unittests/Format/FormatTest.cpp

22410

Can you assert the cases it won't do (given your description it seem like it would do it for all before ) cases)

int a = (int
) b;

I'm pretty sure it won't but lets me explicit in the tests

return (true
);

void foo(
);

etc..

[csmulhern]

clang/unittests/Format/FormatTest.cpp

22410

Done.

[MyDeveloperDay]

clang/unittests/Format/FormatTest.cpp

22414

can't this just be verifyFormat but with 3 arguments?

[csmulhern]

clang/unittests/Format/FormatTest.cpp

22414

Isn't that true of all the EXPECT_EQ cases (as long as the expected code is stable)? This is my first contribution and I'm basically cargo cutting what I see as the prevailing style (verifyFormat for the one param / two param case, EXPECT_EQ for the three param case). I can imagine that verifyFormat didn't support the three parameter case before and so the EXPECT_EQ usage is legacy code. Should I convert everything to use verifyFormat?

[MyDeveloperDay]

we prefer verifyFormat because it does some additional checks to ensure if the code is altered it still goes to how you expect.

[csmulhern]

clang/unittests/Format/FormatTest.cpp

22414

Cool. I've updated the code accordingly.

[MyDeveloperDay]

This seems ok, might be worth adding a release note

[csmulhern]

In D109557#3011516, @MyDeveloperDay wrote:

This seems ok, might be worth adding a release note

Done.

I don't have commit access, so please submit this change on my behalf.

Thanks for all the help!

[MyDeveloperDay]

LGTM

[MyDeveloperDay]

We would need your name and email address to commit this for you in the form git commit --amend --author="John Doe <[e-mail]>"

See https://llvm.org/docs/DeveloperPolicy.html

[csmulhern]

In D109557#3019847, @MyDeveloperDay wrote:

We would need your name and email address to commit this for you in the form git commit --amend --author="John Doe <[e-mail]>"

See https://llvm.org/docs/DeveloperPolicy.html

Sure, it's Cameron Mulhern <[e-mail]>.

[MyDeveloperDay]

FYI, this is a very aggressive change, I highly recommend you run this over a large code base before landing. to double check, here is one slight oddity which I cannot determine if its correct or not.

void foo() {
    if (quitelongarg != (alsolongarg - 1)) { // ABC is a very longgggggggggggg comment
      return;
    }
}

becomes

void foo() {
  if (quitelongarg != (alsolongarg - 1)
  ) { // ABC is a very longgggggggggggg comment
    return;
  }
}

BasedOnStyle: LLVM
BreakBeforeClosingParen: true

That might be what you expect but I wasn't quite sure

[HazardyKnusperkeks]

In D109557#3021312, @MyDeveloperDay wrote:

FYI, this is a very aggressive change, I highly recommend you run this over a large code base before landing. to double check, here is one slight oddity which I cannot determine if its correct or not.

void foo() {
    if (quitelongarg != (alsolongarg - 1)) { // ABC is a very longgggggggggggg comment
      return;
    }
}

becomes

void foo() {
  if (quitelongarg != (alsolongarg - 1)
  ) { // ABC is a very longgggggggggggg comment
    return;
  }
}

BasedOnStyle: LLVM
BreakBeforeClosingParen: true

That might be what you expect but I wasn't quite sure

That is at least not what is covered in the tests or documentation. I would think it only applies to function declarations and invocations.
So either adapt documentation and test coverage, or fix behavior (in which case the tests should be extended as well).

[gracicot]

In D109557#3021667, @HazardyKnusperkeks wrote:

In D109557#3021312, @MyDeveloperDay wrote:

FYI, this is a very aggressive change, I highly recommend you run this over a large code base before landing. to double check, here is one slight oddity which I cannot determine if its correct or not.

void foo() {
    if (quitelongarg != (alsolongarg - 1)) { // ABC is a very longgggggggggggg comment
      return;
    }
}

becomes

void foo() {
  if (quitelongarg != (alsolongarg - 1)
  ) { // ABC is a very longgggggggggggg comment
    return;
  }
}

BasedOnStyle: LLVM
BreakBeforeClosingParen: true

That might be what you expect but I wasn't quite sure

That is at least not what is covered in the tests or documentation. I would think it only applies to function declarations and invocations.
So either adapt documentation and test coverage, or fix behavior (in which case the tests should be extended as well).

I'm waiting for this patch to finally use clang format in our code and we apply this style to all braces. This include ifs, for loops, and all control flow.

We use this style specifically in many places in our code:

if (
       longCondition1
    or longCondition2
    or longCondition3
) {
    // code
}

The you quoted would, in my mind, be formatted like this:

void foo() {
    if (
        quitelongarg != (alsolongarg - 1)
    ) { // ABC is a very longgggggggggggg comment
        return;
    }
}

This is because I don't allow breaking the closing paren without breaking after the opening paren, but this might be only my own style.

[MyDeveloperDay]

I stuggle to see that

if (
        quitelongarg != (alsolongarg - 1)
    )

is correct, I mean 3 lines for a 1 line if seems like this is something different, its like

auto string = std::string(
);

This just doesn't seem correct for empty functions

[MyDeveloperDay]

The you quoted would, in my mind, be formatted like this:

void foo() {
    if (
        quitelongarg != (alsolongarg - 1)
    ) { // ABC is a very longgggggggggggg comment
        return;
    }
}

This is because I don't allow breaking the closing paren without breaking after the opening paren, but this might be only my own style.

Yes, this is not what you are going to get with this revision, we need to decide if that is what is expected

[csmulhern]

In D109557#3063563, @MyDeveloperDay wrote:

The you quoted would, in my mind, be formatted like this:

void foo() {
    if (
        quitelongarg != (alsolongarg - 1)
    ) { // ABC is a very longgggggggggggg comment
        return;
    }
}

This is because I don't allow breaking the closing paren without breaking after the opening paren, but this might be only my own style.

Yes, this is not what you are going to get with this revision, we need to decide if that is what is expected

Sorry for the delay in responding. I have not had the free time recently to address this feedback, but hope to get to it soon. I appreciate this formatting case being called out, and agree with the formatting proposed here. I will update the revision accordingly.

[MyDeveloperDay]

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

if (
    ^

[MyDeveloperDay]

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()
)
....

[MyDeveloperDay]

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>

[csmulhern]

In 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, @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 AlignAfterOpenBracket 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, @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.

[csmulhern]

Absent feedback, I have gone ahead and added a BlockIndent option to AlignAfterOpenBracket.

Please take a look.

[HazardyKnusperkeks]

clang/docs/ReleaseNotes.rst

227 ↗

(On Diff #381765)

Add a new line before.

clang/lib/Format/TokenAnnotator.cpp

4205–4210

I'm not sure if this is sufficient. Before we returned always false. Shouldn't we use 2 ifs here?

[csmulhern]

Any further feedback on the latest iteration?

[HazardyKnusperkeks]

In D109557#3066854, @csmulhern wrote:

In 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 AlignAfterOpenBracket 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.

So we would still have the chance for a breaking if condition? There should be a test to document that! But I'm also not sure this should happen, but lets see, nobody is forced to chose BlockIndent.

Otherwise this is fine for me.

clang/include/clang/Format/Format.h

95

Maybe highlight this with the warning block?
Or correct it. ;)

clang/lib/Format/ContinuationIndenter.cpp

949

Remove the braces

clang/unittests/Format/FormatTest.cpp

22349

How does it behave without these?

22456

I think you should add a test without the comment.

[owenpan]

clang/lib/Format/ContinuationIndenter.cpp

949

I think the braces should not be removed here. The LLVM Coding Standards says "braces should be used when a single-statement body is complex enough" which is rather subjective, but line wrapping is an objective metric and perhaps the only one we can apply.

[csmulhern]

clang/lib/Format/ContinuationIndenter.cpp

949

I'm not sure either way, but the prevailing style in this file seems like it would support having no braces, so I went ahead and made the change.

clang/unittests/Format/FormatTest.cpp

22349

It should put the closing parenthesis on a newline as long as it was opened with a newline. I've added additional test cases.

[csmulhern]

I don't have commit access, so please submit this change on my behalf.

Thanks for all your help on landing this!

[csmulhern]

Is there anything outstanding you'd like me to address, or can we move ahead with committing this?

[HazardyKnusperkeks]

Actually no. I think no one just had thought about it. :)
I can try to commit it next week, this one I'm rather busy. Maybe someone else might do it before that.

[fdwr]

I think no one just had thought about it. :)

@HazardyKnusperkeks Happy new year. Just in case it gets forgotten, I'm squeaking on behalf of a few stackoverflowers 😁 (#52158077, #50689027). Thanks.

[HazardyKnusperkeks]

In D109557#3242274, @fdwr wrote:

I think no one just had thought about it. :)

@HazardyKnusperkeks Happy new year. Just in case it gets forgotten, I'm squeaking on behalf of a few stackoverflowers 😁 (#52158077, #50689027). Thanks.

You're completely correct, it went under. I'll definitely commit this weekend. :)

[HazardyKnusperkeks]

Could you please rebase the patch? I promise I will commit it very shortly afterwards.

[csmulhern]

In D109557#3247167, @HazardyKnusperkeks wrote:

Could you please rebase the patch? I promise I will commit it very shortly afterwards.

Done.

clang/docs/ClangFormatStyleOptions.rst

.. code-block:: c++
       void bar() { foo(true); }
       } // namespace N
 
   * ``BS_Custom`` (in configuration: ``Custom``)
     Configure each individual brace in `BraceWrapping`.
 
 
 
+**BreakBeforeClosingParen** (``CloseBracketAlignmentStyle``)
+  If ``true``, closing parentheses will be placed on a new line.
+
+  This option is only used if there was also a newline after the
+  beginning left paren.
+
+  .. code-block:: c++
+
+    true:
+     someLongFunction(
+         argument1, argument2
+     )
+
+    false:
+     someLongFunction(
+         argument1, argument2)
+
 **BreakBeforeConceptDeclarations** (``bool``)
   If ``true``, concept will be placed on a new line.
 
   .. code-block:: c++
 
     true:
      template<typename T>
      concept ...

clang/include/clang/Format/Format.h

struct FormatStyle {
   /// If ``true``, horizontally aligns arguments after an open bracket.
   ///
   /// This applies to round brackets (parentheses), angle brackets and square
   /// brackets.
   BracketAlignmentStyle AlignAfterOpenBracket;
 
   /// Different style for aligning array initializers.
   enum ArrayInitializerAlignmentStyle {
     /// Align array column and left justify the columns e.g.:
     /// \code
     ///   struct test demo[] =
     ///   {
     ///       {56, 23,    "hello"},
     ///       {-1, 93463, "world"},
     ///       {7,  5,     "!!"   }
     ///   };
     /// \endcode

   ///   BreakBeforeBraces: Custom
   ///   BraceWrapping:
   ///     AfterEnum: true
   ///     AfterStruct: false
   ///     SplitEmptyFunction: false
   /// \endcode
   BraceWrappingFlags BraceWrapping;
 
+  /// If ``true``, closing parentheses will be placed on a new line.
+  ///
+  /// This option is used only if there also was a newline after the
+  /// beginning left paren.
+  ///
+  /// \code
+  ///   true:
+  ///    someLongFunction(
+  ///        argument1, argument2
+  ///    )
+  ///
+  ///   false:
+  ///    someLongFunction(
+  ///        argument1, argument2)
+  /// \endcode
+  bool BreakBeforeClosingParen;
+
   /// If ``true``, concept will be placed on a new line.
   /// \code
   ///   true:
   ///    template<typename T>
   ///    concept ...
   ///
   ///   false:
   ///    template<typename T> concept ...

clang/lib/Format/ContinuationIndenter.h

private:
   const RawStringFormatStyleManager RawStringFormats;
 };
 
 struct ParenState {
   ParenState(const FormatToken *Tok, unsigned Indent, unsigned LastSpace,
              bool AvoidBinPacking, bool NoLineBreak)
       : Tok(Tok), Indent(Indent), LastSpace(LastSpace),
         NestedBlockIndent(Indent), IsAligned(false),
-        BreakBeforeClosingBrace(false), AvoidBinPacking(AvoidBinPacking),
-        BreakBeforeParameter(false), NoLineBreak(NoLineBreak),
-        NoLineBreakInOperand(false), LastOperatorWrapped(true),
-        ContainsLineBreak(false), ContainsUnwrappedBuilder(false),
-        AlignColons(true), ObjCSelectorNameFound(false),
-        HasMultipleNestedBlocks(false), NestedBlockInlined(false),
-        IsInsideObjCArrayLiteral(false), IsCSharpGenericTypeConstraint(false),
-        IsChainedConditional(false), IsWrappedConditional(false),
-        UnindentOperator(false) {}
+        BreakBeforeClosingBrace(false), BreakBeforeClosingParen(false),
+        AvoidBinPacking(AvoidBinPacking), BreakBeforeParameter(false),
+        NoLineBreak(NoLineBreak), NoLineBreakInOperand(false),
+        LastOperatorWrapped(true), ContainsLineBreak(false),
+        ContainsUnwrappedBuilder(false), AlignColons(true),
+        ObjCSelectorNameFound(false), HasMultipleNestedBlocks(false),
+        NestedBlockInlined(false), IsInsideObjCArrayLiteral(false),
+        IsCSharpGenericTypeConstraint(false), IsChainedConditional(false),
+        IsWrappedConditional(false), UnindentOperator(false) {}
 
   /// \brief The token opening this parenthesis level, or nullptr if this level
   /// is opened by fake parenthesis.
   ///
   /// Not considered for memoization as it will always have the same value at
   /// the same token.
   const FormatToken *Tok;
 
struct ParenState {
 
   /// Whether a newline needs to be inserted before the block's closing
   /// brace.
   ///
   /// We only want to insert a newline before the closing brace if there also
   /// was a newline after the beginning left brace.
   bool BreakBeforeClosingBrace : 1;
 
+  /// Whether a newline needs to be inserted before the block's closing
+  /// paren.
+  ///
+  /// We only want to insert a newline before the closing paren if there also
+  /// was a newline after the beginning left paren.
+  bool BreakBeforeClosingParen : 1;
+
   /// Avoid bin packing, i.e. multiple parameters/elements on multiple
   /// lines, in this context.
   bool AvoidBinPacking : 1;
 
   /// Break after the next comma (or all the commas in this context if
   /// \c AvoidBinPacking is \c true).
   bool BreakBeforeParameter : 1;
 
bool operator<(const ParenState &Other) const {
     if (NestedBlockIndent != Other.NestedBlockIndent)
       return NestedBlockIndent < Other.NestedBlockIndent;
     if (FirstLessLess != Other.FirstLessLess)
       return FirstLessLess < Other.FirstLessLess;
     if (IsAligned != Other.IsAligned)
       return IsAligned;
     if (BreakBeforeClosingBrace != Other.BreakBeforeClosingBrace)
       return BreakBeforeClosingBrace;
+    if (BreakBeforeClosingParen != Other.BreakBeforeClosingParen)
+      return BreakBeforeClosingParen;
     if (QuestionColumn != Other.QuestionColumn)
       return QuestionColumn < Other.QuestionColumn;
     if (AvoidBinPacking != Other.AvoidBinPacking)
       return AvoidBinPacking;
     if (BreakBeforeParameter != Other.BreakBeforeParameter)
       return BreakBeforeParameter;
     if (NoLineBreak != Other.NoLineBreak)
       return NoLineBreak;

clang/lib/Format/ContinuationIndenter.cpp

if (Style.BraceWrapping.BeforeLambdaBody && Current.CanBreakBefore &&
     auto LambdaBodyLength = getLengthToMatchingParen(Current, State.Stack);
     return (LambdaBodyLength > getColumnLimit(State));
   }
   if (Current.MustBreakBefore || Current.is(TT_InlineASMColon))
     return true;
   if (State.Stack.back().BreakBeforeClosingBrace &&
       Current.closesBlockOrBlockTypeList(Style))
     return true;
+  if (State.Stack.back().BreakBeforeClosingParen && Current.is(tok::r_paren))
+    return true;
   if (Previous.is(tok::semi) && State.LineContainsContinuedForLoopSection)
     return true;
   if (Style.Language == FormatStyle::LK_ObjC &&
       Style.ObjCBreakBeforeNestedBlockParam &&
       Current.ObjCSelectorNameParts > 1 &&
       Current.startsSequence(TT_SelectorName, tok::colon, tok::caret)) {
     return true;
   }
unsigned ContinuationIndenter::addTokenOnNewLine(LineState &State,
 
   // If we break after { or the [ of an array initializer, we should also break
   // before the corresponding } or ].
   if (PreviousNonComment &&
       (PreviousNonComment->isOneOf(tok::l_brace, TT_ArrayInitializerLSquare) ||
        opensProtoMessageField(*PreviousNonComment, Style)))
     State.Stack.back().BreakBeforeClosingBrace = true;
 
+  if (PreviousNonComment && PreviousNonComment->is(tok::l_paren)) {
+    State.Stack.back().BreakBeforeClosingParen = Style.BreakBeforeClosingParen;
+  }
+
   if (State.Stack.back().AvoidBinPacking) {
     // If we are breaking after '(', '{', '<', or this is the break after a ':'
     // to start a member initializater list in a constructor, this should not
     // be considered bin packing unless the relevant AllowAll option is false or
     // this is a dict/object literal.
     bool PreviousIsBreakingCtorInitializerColon =
         Previous.is(TT_CtorInitializerColon) &&
         Style.BreakConstructorInitializers == FormatStyle::BCIS_AfterColon;
unsigned ContinuationIndenter::getNewLineColumn(const LineState &State) {
   //       a,
   //     ) {
   //       code(); //
   //     }
   if (Current.is(tok::r_paren) && State.Stack.size() > 1 &&
       (!Current.Next ||
        Current.Next->isOneOf(tok::semi, tok::kw_const, tok::l_brace)))
     return State.Stack[State.Stack.size() - 2].LastSpace;
+  if (Style.BreakBeforeClosingParen && Current.is(tok::r_paren) &&
+      State.Stack.size() > 1)
+    return State.Stack[State.Stack.size() - 2].LastSpace;
   if (NextNonComment->is(TT_TemplateString) && NextNonComment->closesScope())
     return State.Stack[State.Stack.size() - 2].LastSpace;
   if (Current.is(tok::identifier) && Current.Next &&
       (Current.Next->is(TT_DictLiteral) ||
        ((Style.Language == FormatStyle::LK_Proto ||
          Style.Language == FormatStyle::LK_TextProto) &&
         Current.Next->isOneOf(tok::less, tok::l_brace))))
     return State.Stack.back().Indent;

clang/lib/Format/Format.cpp

static void mapping(IO &IO, FormatStyle &Style) {
     IO.mapOptional("AlwaysBreakTemplateDeclarations",
                    Style.AlwaysBreakTemplateDeclarations);
     IO.mapOptional("AttributeMacros", Style.AttributeMacros);
     IO.mapOptional("BinPackArguments", Style.BinPackArguments);
     IO.mapOptional("BinPackParameters", Style.BinPackParameters);
     IO.mapOptional("BraceWrapping", Style.BraceWrapping);
     IO.mapOptional("BreakBeforeBinaryOperators",
                    Style.BreakBeforeBinaryOperators);
+    IO.mapOptional("BreakBeforeClosingParen", Style.BreakBeforeClosingParen);
     IO.mapOptional("BreakBeforeConceptDeclarations",
                    Style.BreakBeforeConceptDeclarations);
     IO.mapOptional("BreakBeforeBraces", Style.BreakBeforeBraces);
 
     bool BreakBeforeInheritanceComma = false;
     IO.mapOptional("BreakBeforeInheritanceComma", BreakBeforeInheritanceComma);
     IO.mapOptional("BreakInheritanceList", Style.BreakInheritanceList);
     // If BreakBeforeInheritanceComma was specified but
FormatStyle getLLVMStyle(FormatStyle::LanguageKind Language) {
   LLVMStyle.AlwaysBreakAfterReturnType = FormatStyle::RTBS_None;
   LLVMStyle.AlwaysBreakAfterDefinitionReturnType = FormatStyle::DRTBS_None;
   LLVMStyle.AlwaysBreakBeforeMultilineStrings = false;
   LLVMStyle.AlwaysBreakTemplateDeclarations = FormatStyle::BTDS_MultiLine;
   LLVMStyle.AttributeMacros.push_back("__capability");
   LLVMStyle.BinPackArguments = true;
   LLVMStyle.BinPackParameters = true;
   LLVMStyle.BreakBeforeBinaryOperators = FormatStyle::BOS_None;
+  LLVMStyle.BreakBeforeClosingParen = false;
   LLVMStyle.BreakBeforeConceptDeclarations = true;
   LLVMStyle.BreakBeforeTernaryOperators = true;
   LLVMStyle.BreakBeforeBraces = FormatStyle::BS_Attach;
   LLVMStyle.BraceWrapping = {/*AfterCaseLabel=*/false,
                              /*AfterClass=*/false,
                              /*AfterControlStatement=*/FormatStyle::BWACS_Never,
                              /*AfterEnum=*/false,
                              /*AfterFunction=*/false,

clang/lib/Format/TokenAnnotator.cpp

if (Left.is(tok::l_paren) &&
       Left.isOneOf(TT_AttributeParen, TT_TypeDeclarationParen))
     return false;
   if (Left.is(tok::l_paren) && Left.Previous &&
       (Left.Previous->isOneOf(TT_BinaryOperator, TT_CastRParen)))
     return false;
   if (Right.is(TT_ImplicitStringLiteral))
     return false;
 
-  if (Right.is(tok::r_paren) || Right.is(TT_TemplateCloser))
+  if (Right.is(tok::r_paren))
+    return Style.BreakBeforeClosingParen;
+  if (Right.is(TT_TemplateCloser))
     return false;
   if (Right.is(tok::r_square) && Right.MatchingParen &&
       Right.MatchingParen->is(TT_LambdaLSquare))
     return false;
 
   // We only break before r_brace if there was a corresponding break before
   // the l_brace, which is tracked by BreakBeforeClosingBrace.
   if (Right.is(tok::r_brace))
     return Right.MatchingParen && Right.MatchingParen->is(BK_Block);
 
   // Allow breaking after a trailing annotation, e.g. after a method
   // declaration.
   if (Left.is(TT_TrailingAnnotation))
     return !Right.isOneOf(tok::l_brace, tok::semi, tok::equal, tok::l_paren,
                           tok::less, tok::coloncolon);
 
   if (Right.is(tok::kw___attribute) ||
       (Right.is(tok::l_square) && Right.is(TT_AttributeSquare)))
     return !Left.is(TT_AttributeSquare);
 
   if (Left.is(tok::identifier) && Right.is(tok::string_literal))
     return true;
 
   if (Right.is(tok::identifier) && Right.Next && Right.Next->is(TT_DictLiteral))
     return true;

clang/unittests/Format/FormatTest.cpp

TEST_F(FormatTest, ParsesConfigurationBools) {
   CHECK_PARSE_BOOL(AllowAllArgumentsOnNextLine);
   CHECK_PARSE_BOOL(AllowAllParametersOfDeclarationOnNextLine);
   CHECK_PARSE_BOOL(AllowShortCaseLabelsOnASingleLine);
   CHECK_PARSE_BOOL(AllowShortEnumsOnASingleLine);
   CHECK_PARSE_BOOL(AllowShortLoopsOnASingleLine);
   CHECK_PARSE_BOOL(BinPackArguments);
   CHECK_PARSE_BOOL(BinPackParameters);
   CHECK_PARSE_BOOL(BreakAfterJavaFieldAnnotations);
+  CHECK_PARSE_BOOL(BreakBeforeClosingParen);
   CHECK_PARSE_BOOL(BreakBeforeConceptDeclarations);
   CHECK_PARSE_BOOL(BreakBeforeTernaryOperators);
   CHECK_PARSE_BOOL(BreakStringLiterals);
   CHECK_PARSE_BOOL(CompactNamespaces);
   CHECK_PARSE_BOOL(DeriveLineEnding);
   CHECK_PARSE_BOOL(DerivePointerAlignment);
   CHECK_PARSE_BOOL_FIELD(DerivePointerAlignment, "DerivePointerBinding");
   CHECK_PARSE_BOOL(DisableFormat);
constexpr StringRef Code =
       "  // We already did this with multi line comments and strings. Just to "
       "check if everything is working.\n"
       "  const std::string SmallString = \"Hello World\";\n"
       "  // Small line comment\n"
       "  return String.size() > SmallString.size();\n"
       "}";
   EXPECT_EQ(Code, format(Code, Style));
 }
+
+TEST_F(FormatTest, BreakBeforeClosingParen) {
+  auto Style = getLLVMStyle();
+
+  StringRef Short = "functionCall(paramA, paramB, paramC);\n"
+                    "void functionDecl(int a, int b, int c);";
+
+  StringRef Medium = "functionCall(paramA, paramB, paramC, paramD, paramE, "
+                     "paramF, paramG, paramH, paramI);\n"
+                     "void functionDecl(int argumentA, int argumentB, int "
+                     "argumentC, int argumentD, int argumentE);";
+
+  verifyFormat(Short, Style);
+
+  StringRef NoBreak = "functionCall(paramA, paramB, paramC, paramD, paramE, "
+                      "paramF, paramG, paramH,\n"
+                      "             paramI);\n"
+                      "void functionDecl(int argumentA, int argumentB, int "
+                      "argumentC, int argumentD,\n"
+                      "                  int argumentE);";
+
+  EXPECT_EQ(NoBreak, format(Medium, Style));
+  EXPECT_EQ(NoBreak, format("functionCall(\n"
+                            "    paramA,\n"
+                            "    paramB,\n"
+                            "    paramC,\n"
+                            "    paramD,\n"
+                            "    paramE,\n"
+                            "    paramF,\n"
+                            "    paramG,\n"
+                            "    paramH,\n"
+                            "    paramI\n"
+                            ");\n"
+                            "void functionDecl(\n"
+                            "    int argumentA,\n"
+                            "    int argumentB,\n"
+                            "    int argumentC,\n"
+                            "    int argumentD,\n"
+                            "    int argumentE\n"
+                            ");",
+                            Style));
+
+  verifyFormat("outerFunctionCall(nestedFunctionCall(argument1),\n"
+               "                  nestedLongFunctionCall(argument1, "
+               "argument2, argument3,\n"
+               "                                         argument4, "
+               "argument5));",
+               Style);
+
+  Style.BreakBeforeClosingParen = true;
+
+  verifyFormat(Short, Style);
+
+  EXPECT_EQ(NoBreak, format(Medium, Style));
+
+  Style.AllowAllArgumentsOnNextLine = false;
+  Style.AllowAllConstructorInitializersOnNextLine = false;
+  Style.AllowAllParametersOfDeclarationOnNextLine = false;
+
+  verifyFormat(Short, Style);
+
+  EXPECT_EQ(NoBreak, format(Medium, Style));
+
+  Style.AlignAfterOpenBracket = FormatStyle::BAS_AlwaysBreak;
+
+  verifyFormat(Short, Style);
+
+  EXPECT_EQ("functionCall(\n"
+            "    paramA, paramB, paramC, paramD, paramE, paramF, paramG, "
+            "paramH, paramI\n"
+            ");\n"
+            "void functionDecl(\n"
+            "    int argumentA, int argumentB, int argumentC, int argumentD, "
+            "int argumentE\n"
+            ");",
+            format(Medium, Style));
+
+  Style.BinPackArguments = false;
+  Style.BinPackParameters = false;
+
+  verifyFormat(Short, Style);
+
+  EXPECT_EQ("functionCall(\n"
+            "    paramA,\n"
+            "    paramB,\n"
+            "    paramC,\n"
+            "    paramD,\n"
+            "    paramE,\n"
+            "    paramF,\n"
+            "    paramG,\n"
+            "    paramH,\n"
+            "    paramI\n"
+            ");\n"
+            "void functionDecl(\n"
+            "    int argumentA,\n"
+            "    int argumentB,\n"
+            "    int argumentC,\n"
+            "    int argumentD,\n"
+            "    int argumentE\n"
+            ");",
+            format(Medium, Style));
+
+  verifyFormat("outerFunctionCall(\n"
+               "    nestedFunctionCall(argument1),\n"
+               "    nestedLongFunctionCall(\n"
+               "        argument1,\n"
+               "        argument2,\n"
+               "        argument3,\n"
+               "        argument4,\n"
+               "        argument5\n"
+               "    )\n"
+               ");",
+               Style);
+
+  verifyFormat("int a = (int)b;", Style);
+
+  EXPECT_EQ("int a = (int)b;", format("int a = (\n"
+                                      "    int\n"
+                                      ") b;",
+                                      Style));
+
+  verifyFormat("return (true);", Style);
+
+  EXPECT_EQ("return (true);", format("return (\n"
+                                     "    true\n"
+                                     ");",
+                                     Style));
+
+  verifyFormat("void foo();", Style);
+
+  EXPECT_EQ("void foo();", format("void foo(\n"
+                                  ");",
+                                  Style));
+
+  verifyFormat("void foo() {}", Style);
+
+  EXPECT_EQ("void foo() {}", format("void foo(\n"
+                                    ") {\n"
+                                    "}",
+                                    Style));
+
+  verifyFormat("auto string = std::string();", Style);
+
+  EXPECT_EQ("auto string = std::string();",
+            format("auto string = std::string(\n"
+                   ");",
+                   Style));
+
+  verifyFormat("void (*functionPointer)() = nullptr;", Style);
+
+  EXPECT_EQ("void (*functionPointer)() = nullptr;",
+            format("void (\n"
+                   "    *functionPointer\n"
+                   ")\n"
+                   "(\n"
+                   ") = nullptr;",
+                   Style));
+}
+
 } // namespace
 } // namespace format
 } // namespace clang