Page MenuHomePhabricator

Adds a BlockIndent option to AlignAfterOpenBracket
AcceptedPublic

Authored by csmulhern on Sep 9 2021, 5:37 PM.
Tags
  • Restricted Project
  • Restricted Project
Tokens
"Party Time" token, awarded by aminya."Love" token, awarded by maksim."Pterodactyl" token, awarded by xgupta."Like" token, awarded by jeevcat."Love" token, awarded by fdwr."Like" token, awarded by reupen."Love" token, awarded by kring.

Details

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.

Diff Detail

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
csmulhern retitled this revision from Adds an AlignCloseBracket option to Adds a BreakBeforeClosingParen option.
csmulhern edited the summary of this revision. (Show Details)

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.

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

Yeah I thought that too.

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

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 updated this revision to Diff 372829.Sep 15 2021, 4:07 PM
csmulhern marked an inline comment as done.
csmulhern added inline comments.
clang/unittests/Format/FormatTest.cpp
22301

Done.

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

MyDeveloperDay added inline comments.Sep 16 2021, 4:17 AM
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 updated this revision to Diff 372979.Sep 16 2021, 9:43 AM
csmulhern marked an inline comment as done.
csmulhern marked an inline comment as done.
csmulhern added inline comments.
clang/unittests/Format/FormatTest.cpp
22410

Done.

clang/unittests/Format/FormatTest.cpp
22414

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

csmulhern marked an inline comment as done.Sep 18 2021, 7:17 AM
csmulhern added inline comments.
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?

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

csmulhern updated this revision to Diff 373711.Sep 20 2021, 2:26 PM
csmulhern marked an inline comment as done.
csmulhern added inline comments.
clang/unittests/Format/FormatTest.cpp
22414

Cool. I've updated the code accordingly.

MyDeveloperDay accepted this revision.Sep 21 2021, 1:37 AM

This seems ok, might be worth adding a release note

This revision is now accepted and ready to land.Sep 21 2021, 1:37 AM
csmulhern updated this revision to Diff 373877.Sep 21 2021, 5:39 AM

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!

We would need your name and email address to commit this for you in the form git commit --amend --author="John Doe <jdoe@llvm.org>"

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

We would need your name and email address to commit this for you in the form git commit --amend --author="John Doe <jdoe@llvm.org>"

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

Sure, it's Cameron Mulhern <csmulhern@gmail.com>.

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 requested changes to this revision.Sep 24 2021, 1:33 PM

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

This revision now requires changes to proceed.Sep 24 2021, 1:33 PM

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.

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

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

kring awarded a token.Oct 14 2021, 3:25 AM
kring added a subscriber: kring.

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.

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

if (
    ^

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()
)
....
fdwr awarded a token.Oct 14 2021, 10:49 AM
fdwr added a subscriber: fdwr.
wmmc88 added a subscriber: wmmc88.Oct 14 2021, 4:17 PM
fdwr mentioned this in fdwr.Oct 14 2021, 5:07 PM
jeevcat added a subscriber: jeevcat.

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>
Godin added a subscriber: Godin.Oct 15 2021, 4:27 AM
csmulhern added a comment.EditedOct 15 2021, 7:45 AM

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

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.

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.

maksim added a subscriber: maksim.
csmulhern retitled this revision from Adds a BreakBeforeClosingParen option to Adds a BlockIndent option to AlignAfterOpenBracket.Oct 23 2021, 4:14 PM
csmulhern edited the summary of this revision. (Show Details)
csmulhern updated this revision to Diff 381765.Oct 23 2021, 4:17 PM

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

Please take a look.

clang/docs/ReleaseNotes.rst
227 ↗(On Diff #381765)

Add a new line before.

clang/lib/Format/TokenAnnotator.cpp
4207–4212

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

csmulhern updated this revision to Diff 381974.Oct 25 2021, 6:47 AM
csmulhern marked 2 inline comments as done.

Any further feedback on the latest iteration?

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
100

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

clang/lib/Format/ContinuationIndenter.cpp
951

Remove the braces

clang/unittests/Format/FormatTest.cpp
22453

How does it behave without these?

22560

I think you should add a test without the comment.

owenpan added inline comments.
clang/lib/Format/ContinuationIndenter.cpp
951

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 updated this revision to Diff 387025.Sat, Nov 13, 8:01 AM
csmulhern marked 4 inline comments as done.Sat, Nov 13, 8:06 AM
csmulhern added inline comments.
clang/lib/Format/ContinuationIndenter.cpp
951

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
22453

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

This revision is now accepted and ready to land.Sat, Nov 13, 11:38 AM
csmulhern marked an inline comment as done.Sat, Nov 13, 12:27 PM

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

Thanks for all your help on landing this!