Page MenuHomePhabricator

Adds a BreakBeforeClosingParen option
Needs RevisionPublic

Authored by csmulhern on Sep 9 2021, 5:37 PM.
Tags
  • Restricted Project
  • Restricted Project
Tokens
"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 can be used to control the breaking of closing parentheses.

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.

Diff Detail

Unit TestsFailed

TimeTest
80 msx64 debian > ORC-x86_64-linux.TestCases/Linux/x86-64::trivial-cxa-atexit.S
Script: -- : 'RUN: at line 3'; /var/lib/buildkite-agent/builds/llvm-project/build/./bin/clang -m64 -c -o /var/lib/buildkite-agent/builds/llvm-project/build/projects/compiler-rt/test/orc/X86_64LinuxConfig/TestCases/Linux/x86-64/Output/trivial-cxa-atexit.S.tmp /var/lib/buildkite-agent/builds/llvm-project/compiler-rt/test/orc/TestCases/Linux/x86-64/trivial-cxa-atexit.S
90 msx64 debian > ORC-x86_64-linux.TestCases/Linux/x86-64::trivial-static-initializer.S
Script: -- : 'RUN: at line 7'; /var/lib/buildkite-agent/builds/llvm-project/build/./bin/clang -m64 -c -o /var/lib/buildkite-agent/builds/llvm-project/build/projects/compiler-rt/test/orc/X86_64LinuxConfig/TestCases/Linux/x86-64/Output/trivial-static-initializer.S.tmp /var/lib/buildkite-agent/builds/llvm-project/compiler-rt/test/orc/TestCases/Linux/x86-64/trivial-static-initializer.S
110 msx64 debian > ORC-x86_64-linux.TestCases/Linux/x86-64::trivial-tls.S
Script: -- : 'RUN: at line 1'; /var/lib/buildkite-agent/builds/llvm-project/build/./bin/clang -m64 -c -o /var/lib/buildkite-agent/builds/llvm-project/build/projects/compiler-rt/test/orc/X86_64LinuxConfig/TestCases/Linux/x86-64/Output/trivial-tls.S.tmp /var/lib/buildkite-agent/builds/llvm-project/compiler-rt/test/orc/TestCases/Linux/x86-64/trivial-tls.S

Event Timeline

csmulhern requested review of this revision.Sep 9 2021, 5:37 PM
csmulhern created this revision.
Herald added a project: Restricted Project. · View Herald TranscriptSep 9 2021, 5:37 PM
Herald added a subscriber: cfe-commits. · View Herald Transcript
MyDeveloperDay added a project: Restricted Project.

Please resubmit the patch with full context

MyDeveloperDay requested changes to this revision.Sep 13 2021, 10:00 AM
This revision now requires changes to proceed.Sep 13 2021, 10:00 AM
csmulhern edited the summary of this revision. (Show Details)Sep 13 2021, 10:07 AM

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

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

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

csmulhern updated this revision to Diff 372364.Sep 13 2021, 4:27 PM

Adds more context to the diff.

Ah sorry about that. Done.

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?

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

Ah sorry about that. Done.

No Problem.

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.

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

Yeah I thought that too.

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.Mon, Sep 20, 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.Tue, Sep 21, 1:37 AM

This seems ok, might be worth adding a release note

This revision is now accepted and ready to land.Tue, Sep 21, 1:37 AM
csmulhern updated this revision to Diff 373877.Tue, Sep 21, 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.Fri, Sep 24, 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.Fri, Sep 24, 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.Thu, Oct 14, 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.Thu, Oct 14, 10:49 AM
fdwr added a subscriber: fdwr.
wmmc88 added a subscriber: wmmc88.Thu, Oct 14, 4:17 PM
fdwr mentioned this in fdwr.Thu, Oct 14, 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.Fri, Oct 15, 4:27 AM
csmulhern added a comment.EditedFri, Oct 15, 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.