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

23169

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

23169

Done.

[HazardyKnusperkeks]

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

[MyDeveloperDay]

clang/unittests/Format/FormatTest.cpp

23278

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

23278

Done.

[MyDeveloperDay]

clang/unittests/Format/FormatTest.cpp

23282

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

[csmulhern]

clang/unittests/Format/FormatTest.cpp

23282

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

23282

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

302

Add a new line before.

clang/lib/Format/TokenAnnotator.cpp

4331–4336

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

950

Remove the braces

clang/unittests/Format/FormatTest.cpp

23218

How does it behave without these?

23325

I think you should add a test without the comment.

[owenpan]

clang/lib/Format/ContinuationIndenter.cpp

950

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

950

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

23218

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

* ``BAS_AlwaysBreak`` (in configuration: ``AlwaysBreak``)
     Always break after an open bracket, if the parameters don't fit
     on a single line, e.g.:
 
     .. code-block:: c++
 
       someLongFunction(
           argument1, argument2);
 
+  * ``BAS_BlockIndent`` (in configuration: ``BlockIndent``)
+    Always break after an open bracket, if the parameters don't fit
+    on a single line. Closing brackets will be placed on a new line.
+    E.g.:
+
+    .. code-block:: c++
+
+      someLongFunction(
+          argument1, argument2
+      )
+
+
+    .. warning:: 
+
+     Note: This currently only applies to parentheses.
+
 
 
 **AlignArrayOfStructures** (``ArrayInitializerAlignmentStyle``) :versionbadge:`clang-format 13`
   if not ``None``, when using initialization for an array of structs
   aligns the fields into columns.
 
   Possible values:
 

clang/docs/ReleaseNotes.rst

- The ``usingType`` matcher is now available and needed to refer to types that
   ``hasDeclaration`` continues to see through the alias and apply to the
   underlying type.
 
 clang-format
 ------------
 
 - Option ``AllowShortEnumsOnASingleLine: false`` has been improved, it now
   correctly places the opening brace according to ``BraceWrapping.AfterEnum``.
 
+- Option ``AlignAfterOpenBracket: BlockIndent`` has been added. If set, it will
+  always break after an open bracket, if the parameters don't fit on a single
+  line. Closing brackets will be placed on a new line.
+
 - Option ``QualifierAlignment`` has been added in order to auto-arrange the
   positioning of specifiers/qualifiers
   `const` `volatile` `static` `inline` `constexpr` `restrict`
   in variable and parameter declarations to be either ``Right`` aligned
   or ``Left`` aligned or ``Custom`` using ``QualifierOrder``.
 
 - Option ``QualifierOrder`` has been added to allow the order
   `const` `volatile` `static` `inline` `constexpr` `restrict`

clang/include/clang/Format/Format.h

enum BracketAlignmentStyle : unsigned char {
     BAS_DontAlign,
     /// Always break after an open bracket, if the parameters don't fit
     /// on a single line, e.g.:
     /// \code
     ///   someLongFunction(
     ///       argument1, argument2);
     /// \endcode
     BAS_AlwaysBreak,
+    /// Always break after an open bracket, if the parameters don't fit
+    /// on a single line. Closing brackets will be placed on a new line.
+    /// E.g.:
+    /// \code
+    ///   someLongFunction(
+    ///       argument1, argument2
+    ///   )
+    /// \endcode
+    ///
+    /// \warning
+    ///  Note: This currently only applies to parentheses.
+    /// \endwarning
+    BAS_BlockIndent,
   };
 
   /// If ``true``, horizontally aligns arguments after an open bracket.
   ///
   /// This applies to round brackets (parentheses), angle brackets and square
   /// brackets.
   /// \version 3.8
   BracketAlignmentStyle AlignAfterOpenBracket;

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;
   }
if (Current.is(TT_SelectorName) &&
     if (Current.LongestObjCSelectorName == 0)
       State.Stack.back().AlignColons = false;
     else if (MinIndent + Current.LongestObjCSelectorName > FirstColonPos)
       State.Stack.back().ColonPos = MinIndent + Current.LongestObjCSelectorName;
     else
       State.Stack.back().ColonPos = FirstColonPos;
   }
 
-  // In "AlwaysBreak" mode, enforce wrapping directly after the parenthesis by
-  // disallowing any further line breaks if there is no line break after the
-  // opening parenthesis. Don't break if it doesn't conserve columns.
-  if (Style.AlignAfterOpenBracket == FormatStyle::BAS_AlwaysBreak &&
+  // In "AlwaysBreak" or "BlockIndent" mode, enforce wrapping directly after the
+  // parenthesis by disallowing any further line breaks if there is no line
+  // break after the opening parenthesis. Don't break if it doesn't conserve
+  // columns.
+  if ((Style.AlignAfterOpenBracket == FormatStyle::BAS_AlwaysBreak ||
+       Style.AlignAfterOpenBracket == FormatStyle::BAS_BlockIndent) &&
       (Previous.isOneOf(tok::l_paren, TT_TemplateOpener, tok::l_square) ||
        (Previous.is(tok::l_brace) && Previous.isNot(BK_Block) &&
         Style.Cpp11BracedListStyle)) &&
       State.Column > getNewLineColumn(State) &&
       (!Previous.Previous || !Previous.Previous->isOneOf(
                                  tok::kw_for, tok::kw_while, tok::kw_switch)) &&
       // Don't do this for simple (no expressions) one-argument function calls
       // as that feels like needlessly wasting whitespace, e.g.:
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.AlignAfterOpenBracket == FormatStyle::BAS_BlockIndent;
+
   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.AlignAfterOpenBracket == FormatStyle::BAS_BlockIndent &&
+      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

struct ScalarEnumerationTraits<FormatStyle::NamespaceIndentationKind> {
   }
 };
 
 template <> struct ScalarEnumerationTraits<FormatStyle::BracketAlignmentStyle> {
   static void enumeration(IO &IO, FormatStyle::BracketAlignmentStyle &Value) {
     IO.enumCase(Value, "Align", FormatStyle::BAS_Align);
     IO.enumCase(Value, "DontAlign", FormatStyle::BAS_DontAlign);
     IO.enumCase(Value, "AlwaysBreak", FormatStyle::BAS_AlwaysBreak);
+    IO.enumCase(Value, "BlockIndent", FormatStyle::BAS_BlockIndent);
 
     // For backward compatibility.
     IO.enumCase(Value, "true", FormatStyle::BAS_Align);
     IO.enumCase(Value, "false", FormatStyle::BAS_DontAlign);
   }
 };
 
 template <>

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(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);
 
+  // We only break before r_paren if we're in a block indented context.
+  if (Right.is(tok::r_paren)) {
+    if (Style.AlignAfterOpenBracket == FormatStyle::BAS_BlockIndent) {
+      return Right.MatchingParen &&
+             !(Right.MatchingParen->Previous &&
+               (Right.MatchingParen->Previous->is(tok::kw_for) ||
+                Right.MatchingParen->Previous->isIf()));
+    }
+
+    return false;
+  }
+
   // 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)))

clang/unittests/Format/FormatTest.cpp

TEST_F(FormatTest, ParsesConfiguration) {
 
   Style.AlignAfterOpenBracket = FormatStyle::BAS_AlwaysBreak;
   CHECK_PARSE("AlignAfterOpenBracket: Align", AlignAfterOpenBracket,
               FormatStyle::BAS_Align);
   CHECK_PARSE("AlignAfterOpenBracket: DontAlign", AlignAfterOpenBracket,
               FormatStyle::BAS_DontAlign);
   CHECK_PARSE("AlignAfterOpenBracket: AlwaysBreak", AlignAfterOpenBracket,
               FormatStyle::BAS_AlwaysBreak);
+  CHECK_PARSE("AlignAfterOpenBracket: BlockIndent", AlignAfterOpenBracket,
+              FormatStyle::BAS_BlockIndent);
   // For backward compatibility:
   CHECK_PARSE("AlignAfterOpenBracket: false", AlignAfterOpenBracket,
               FormatStyle::BAS_DontAlign);
   CHECK_PARSE("AlignAfterOpenBracket: true", AlignAfterOpenBracket,
               FormatStyle::BAS_Align);
 
   Style.AlignEscapedNewlines = FormatStyle::ENAS_Left;
   CHECK_PARSE("AlignEscapedNewlines: DontAlign", AlignEscapedNewlines,
constexpr StringRef Code =
       "  // Small line comment\n"
       "  return String.size() > SmallString.size();\n"
       "}";
   EXPECT_EQ(Code, format(Code, Style));
 }
 
 TEST_F(FormatTest, FormatDecayCopy) {
   // error cases from unit tests
   verifyFormat("foo(auto())");
   verifyFormat("foo(auto{})");
   verifyFormat("foo(auto({}))");
   verifyFormat("foo(auto{{}})");
 
   verifyFormat("foo(auto(1))");
   verifyFormat("foo(auto{1})");
   verifyFormat("foo(new auto(1))");
   verifyFormat("foo(new auto{1})");
TEST_F(FormatTest, Cpp20ModulesSupport) {
   verifyFormat("export int func() {\n"
                "  foo();\n"
                "}",
                Style);
   verifyFormat("export struct {\n"
                "  int foo;\n"
                "};",
                Style);
   verifyFormat("export {\n"
                "  int foo;\n"
                "};",
                Style);
   verifyFormat("export export char const *hello() { return \"hello\"; }");
 
   verifyFormat("import bar;", Style);
   verifyFormat("import foo.bar;", Style);
   verifyFormat("import foo:bar;", Style);

 TEST_F(FormatTest, CoroutineCoAwait) {
   verifyFormat("int x = co_await foo();");
   verifyFormat("int x = (co_await foo());");
   verifyFormat("co_await (42);");
   verifyFormat("void operator co_await(int);");
   verifyFormat("void operator co_await(a);");
   verifyFormat("co_await a;");
   verifyFormat("co_await missing_await_resume{};");
   verifyFormat("co_await a; // comment");
   verifyFormat("void test0() { co_await a; }");
   verifyFormat("co_await co_await co_await foo();");
   verifyFormat("co_await foo().bar();");
   verifyFormat("co_await [this]() -> Task { co_return x; }");
   verifyFormat("co_await [this](int a, int b) -> Task { co_return co_await "
                "foo(); }(x, y);");
 
   FormatStyle Style = getLLVMStyleWithColumns(40);
   verifyFormat("co_await [this](int a, int b) -> Task {\n"
                "  co_return co_await foo();\n"
                "}(x, y);",
                Style);
TEST_F(FormatTest, EmptyShortBlock) {
 
   verifyFormat("try {\n"
                "  doA();\n"
                "} catch (Exception &e) {\n"
                "  e.printStackTrace();\n"
                "}\n",
                Style);
 
   verifyFormat("try {\n"
                "  doA();\n"
                "} catch (Exception &e) {}\n",
                Style);
 }
 
 TEST_F(FormatTest, ShortTemplatedArgumentLists) {
   auto Style = getLLVMStyle();
 
verifyFormat("if (a)\n"
                "c234567890223456789032345678904234567890;",
                "if (a) {\n"
                "  b234567890223456789032345678904234567890 = "
                "c234567890223456789032345678904234567890;\n"
                "}",
                Style);
 }
 
+TEST_F(FormatTest, AlignAfterOpenBracketBlockIndent) {
+  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);";
+
+  verifyFormat(NoBreak, Medium, Style);
+  verifyFormat(NoBreak,
+               "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.AlignAfterOpenBracket = FormatStyle::BAS_BlockIndent;
+
+  verifyFormat(Short, Style);
+  verifyFormat(
+      "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"
+      ");",
+      Medium, Style);
+
+  Style.AllowAllArgumentsOnNextLine = false;
+  Style.AllowAllParametersOfDeclarationOnNextLine = false;
+
+  verifyFormat(Short, Style);
+  verifyFormat(
+      "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"
+      ");",
+      Medium, Style);
+
+  Style.BinPackArguments = false;
+  Style.BinPackParameters = false;
+
+  verifyFormat(Short, Style);
+
+  verifyFormat("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"
+               ");",
+               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);
+  verifyFormat("int a = (int)b;",
+               "int a = (\n"
+               "    int\n"
+               ") b;",
+               Style);
+
+  verifyFormat("return (true);", Style);
+  verifyFormat("return (true);",
+               "return (\n"
+               "    true\n"
+               ");",
+               Style);
+
+  verifyFormat("void foo();", Style);
+  verifyFormat("void foo();",
+               "void foo(\n"
+               ");",
+               Style);
+
+  verifyFormat("void foo() {}", Style);
+  verifyFormat("void foo() {}",
+               "void foo(\n"
+               ") {\n"
+               "}",
+               Style);
+
+  verifyFormat("auto string = std::string();", Style);
+  verifyFormat("auto string = std::string();",
+               "auto string = std::string(\n"
+               ");",
+               Style);
+
+  verifyFormat("void (*functionPointer)() = nullptr;", Style);
+  verifyFormat("void (*functionPointer)() = nullptr;",
+               "void (\n"
+               "    *functionPointer\n"
+               ")\n"
+               "(\n"
+               ") = nullptr;",
+               Style);
+}
+
+TEST_F(FormatTest, AlignAfterOpenBracketBlockIndentIfStatement) {
+  auto Style = getLLVMStyle();
+
+  verifyFormat("if (foo()) {\n"
+               "  return;\n"
+               "}",
+               Style);
+
+  verifyFormat("if (quitelongarg !=\n"
+               "    (alsolongarg - 1)) { // ABC is a very longgggggggggggg "
+               "comment\n"
+               "  return;\n"
+               "}",
+               Style);
+
+  Style.AlignAfterOpenBracket = FormatStyle::BAS_BlockIndent;
+
+  verifyFormat("if (foo()) {\n"
+               "  return;\n"
+               "}",
+               Style);
+
+  verifyFormat("if (quitelongarg !=\n"
+               "    (alsolongarg - 1)) { // ABC is a very longgggggggggggg "
+               "comment\n"
+               "  return;\n"
+               "}",
+               Style);
+}
+
+TEST_F(FormatTest, AlignAfterOpenBracketBlockIndentForStatement) {
+  auto Style = getLLVMStyle();
+
+  verifyFormat("for (int i = 0; i < 5; ++i) {\n"
+               "  doSomething();\n"
+               "}",
+               Style);
+
+  verifyFormat("for (int myReallyLongCountVariable = 0; "
+               "myReallyLongCountVariable < count;\n"
+               "     myReallyLongCountVariable++) {\n"
+               "  doSomething();\n"
+               "}",
+               Style);
+
+  Style.AlignAfterOpenBracket = FormatStyle::BAS_BlockIndent;
+
+  verifyFormat("for (int i = 0; i < 5; ++i) {\n"
+               "  doSomething();\n"
+               "}",
+               Style);
+
+  verifyFormat("for (int myReallyLongCountVariable = 0; "
+               "myReallyLongCountVariable < count;\n"
+               "     myReallyLongCountVariable++) {\n"
+               "  doSomething();\n"
+               "}",
+               Style);
+}
+
 } // namespace
 } // namespace format
 } // namespace clang