Page MenuHomePhabricator

[clang-format] add option for dangling parenthesis
Needs ReviewPublic

Authored by stringham on May 9 2017, 9:47 PM.
Tags
  • Restricted Project
  • Restricted Project
Tokens
"Love" token, awarded by njaldea."Love" token, awarded by glfmn."Mountain of Wealth" token, awarded by aaptho."Love" token, awarded by teohhanhui."Like" token, awarded by quezak."Love" token, awarded by mdrohmann."Love" token, awarded by mmpestorich."Love" token, awarded by mnemotic."Like" token, awarded by msafi."Like" token, awarded by nyaapa."Love" token, awarded by fdwr."Mountain of Wealth" token, awarded by paulreimer."Like" token, awarded by reupen."Like" token, awarded by hyldmo."Like" token, awarded by Rapatas."Like" token, awarded by fiblit."Like" token, awarded by dmcyk."Like" token, awarded by tadeu."Like" token, awarded by DevAlone."Like" token, awarded by sciencemanx."Like" token, awarded by catskul."Like" token, awarded by kc9jud."Like" token, awarded by zroug."Like" token, awarded by VZ."Like" token, awarded by jtbandes.

Details

Summary

This adds an option to clang-format to support dangling parenthesis.

If you have a parameter list like

Foo(
    param1,
    param2,
    param3,
);

clang-format currently only supports putting the closing paren on the same line as param3:

Foo(
    param1,
    param2,
    param3, );

This makes it difficult to see the change from the function signature to the function body, for example:

class Foo extends Bar {
    constructor(
        param1,
        param2,
        param3, ) {
        super(param1, 'x');
        this.param2 = param2;
    }
}

would now be formatted like:

class Foo extends Bar {
    constructor(
        param1,
        param2,
        param3, 
    ) {
        super(param1, 'x');
        this.param2 = param2;
    }
}

and it is much easier to see the difference between the parameter list and the function body.

Diff Detail

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
stringham updated this revision to Diff 98873.May 12 2017, 8:22 PM

Probably all of the examples from the original patch description and later comments should be turned into unit tests.

docs/ClangFormatStyleOptions.rst
953

Have you auto-generated this with docs/tools/dump_format_style.py? There seem to be subtle differences.

include/clang/Format/Format.h
793

I don't think this is a name that anyone will intuitively understand. I understand that the naming is hard here. One thing I am wondering is whether this might ever make sense unless AlignAfterOpenBracket is set to AlwaysBreak?

Unless that option is set, we could have both in the same file:

someFunction(aaaa,
             bbbb);

and

someFunction(
    aaaa, bbbb
);

Is that intended, i.e. are you actively using that? Answering this is important, because it influence whether or not we actually need to add another style option and even how to implement this.

lib/Format/TokenAnnotator.cpp
2655–2657

No braces

Probably all of the examples from the original patch description and later comments should be turned into unit tests.

I would like to add some unit tests for this, but was not able to figure out how to run the unit test suite. Where can I find instructions on how to run the unit tests?

I found out how to build clang-format from a combination of https://chromium.googlesource.com/chromium/src/+/master/docs/updating_clang_format_binaries.md and https://github.com/angular/clang-format#compiling-clang-format

I saw step 10 on http://clang.llvm.org/get_started.html says to run make check-clang from my build directory, but when I try that I get "No rule to make target" errors.

docs/ClangFormatStyleOptions.rst
953

I was not aware the dump_format_style.py existed. I created this manually based on the code around it. I will try running that script.

include/clang/Format/Format.h
793

The name was based on the configuration option that scalafmt has for their automatic scala formatter, they also have an option to have the closing paren on its own line and they call it danglingParentheses. I don't love the name and am open to other options.

That's a good point about AlignAfterOpenBracket being set to AlwaysBreak. In our usage we have that option set, and I'm also unsure if it makes sense without AlwaysBreak.

stringham updated this revision to Diff 99010.May 15 2017, 8:45 AM

Generated the documentation using dump_format_style.py

removed some unnecessary braces.

stringham marked 3 inline comments as done.May 15 2017, 8:46 AM
djasper added inline comments.May 17 2017, 6:14 AM
include/clang/Format/Format.h
793

Hm. I am not sure either. What do you think of extending the BracketAlignmentStyle enum with an AlwaysBreakWithDanglingParenthesis? Or AlwaysBreakAndWrapRParen?

stringham added inline comments.May 30 2017, 7:32 PM
include/clang/Format/Format.h
793

Sorry for the delay in the reply!

That seems like a reasonable solution to me. I believe the structure of the patch would be the same, just changing out the configuration option.

Can you point me to where I should look to figure out how to run the unit tests and let me know if there are other changes you would recommend besides updating configuration options?

@djasper should I move forward with extending the BracketAlignmentStyle option? I'm happy to do it, but I'd like to get the idea signed off on before I spend more time working on it. We've been using a version of clang-format with these changes since May, and we've been happy with it.

Does my current approach seem reasonable, or should I follow a different pattern?

Godin added a subscriber: Godin.May 4 2018, 2:11 PM

Are there any updates on this revision? The team at my company is interested in using this linting rule in C++ code.

I'd also like to note that this is the code style preferred by most modern code formatters that I know of and use:

  • rustfmt for rust code
  • prettier for javascript code
  • black for python code

@stringham Here is what I did to run the unit tests:

cd build
make FormatTests
tools/clang/unittests/Format/FormatTests

@djasper can you give some direction here?

Would you be okay with me extending the BracketAlignmentStyle option?
What do I need to do to get this change merged?

sqrt added a subscriber: sqrt.Sep 11 2018, 6:04 AM
VZ awarded a token.Oct 13 2018, 8:40 AM
VZ added a subscriber: VZ.
kc9jud awarded a token.Nov 8 2018, 3:40 PM
kc9jud added a subscriber: kc9jud.

@djasper bump -- this feature is also really important to our team.

catskul rescinded a token.
catskul awarded a token.
DevAlone added a subscriber: DevAlone.

So, will it be added?

dmcyk awarded a token.Dec 11 2018, 7:44 AM
dmcyk added a subscriber: dmcyk.
reupen added a subscriber: reupen.Dec 11 2018, 12:31 PM
fiblit added a subscriber: fiblit.
Rapatas added a subscriber: Rapatas.
hyldmo added a subscriber: hyldmo.
paulreimer added a subscriber: paulreimer.

This review lack unit tests, please add some for FormatTest to show its use cases, otherwise someone else will come along and break this later

@MyDeveloperDay I am happy to write unit tests and clean up the change set but I don't want to spend more time on this if it has no chance of being merged.

I am looking for some agreement that this feature is worth adding to clang format and then I'd like to know that my general solution is reasonable and agree on what we should call the option.

Currently, we're just using a fork with this patch applied. It would be nice to get the changes merged upstream, but I don't know how to make that happen.

Adding the unit tests lets us see how this option will work in various cases, it will let us understand that its not breaking anything else.

I personally don't like to see revisions like this sit for 2 years with nothing happening, I don't see anything wrong with this that would prevent it going in so I don't understand whats blocking it?,

if you had some tests and a release note I'd give it a LGTM (but as I've said before I'm not the code owner, but someone wanting to address defects and add capabilities. but I think we need to be able to move forward, people will object soon enough if they don't like it.)

Generally I don't understand why clang-format is so reluctant to change anything, As such we don't have many people involved and getting anything done (even defects) is extremely hard.

It looks like you met the criteria, and reviewers have been given ample opportunity to make an objection. the number of subscribers and like tokens would suggest this is wanted,

Please also add a line the in the release notes to say what you are adding. In the absence of any other constructive input all we can do is follow the guidance on doing a review, for what its worth I notice in the rest of LLVM there seems to be a much larger amount of commits that go in even without a review, I'm not sure what makes this area so strict, so reluctant to change especailly when revisions do seem to be reviewed.

Adding the unit tests lets us see how this option will work in various cases, it will let us understand that its not breaking anything else.

I personally don't like to see revisions like this sit for 2 years with nothing happening, I don't see anything wrong with this that would prevent it going in so I don't understand whats blocking it?,

if you had some tests and a release note I'd give it a LGTM (but as I've said before I'm not the code owner, but someone wanting to address defects and add capabilities. but I think we need to be able to move forward, people will object soon enough if they don't like it.)

Generally I don't understand why clang-format is so reluctant to change anything, As such we don't have many people involved and getting anything done (even defects) is extremely hard.

It looks like you met the criteria, and reviewers have been given ample opportunity to make an objection. the number of subscribers and like tokens would suggest this is wanted,

Please also add a line the in the release notes to say what you are adding. In the absence of any other constructive input all we can do is follow the guidance on doing a review, for what its worth I notice in the rest of LLVM there seems to be a much larger amount of commits that go in even without a review, I'm not sure what makes this area so strict, so reluctant to change especailly when revisions do seem to be reviewed.

I don't have any stake here, but i just want to point out that no tool (including clang-format)
will ever support all the things all the people out there will want it to support. But every
new knob is not just a single knob, it needs to work well with all the other existing knobs
(and all of the combination of knob params), and every new knob after that.

It's a snowball effect. Things can (and likely will, unless there is at least a *very* strict testing/quality policy
(which is https://clang.llvm.org/docs/ClangFormatStyleOptions.html#adding-additional-style-options about))
get out of hand real quickly..

Just 2c.

MyDeveloperDay added a comment.EditedMar 10 2019, 6:23 AM

Adding the unit tests lets us see how this option will work in various cases, it will let us understand that its not breaking anything else.

I personally don't like to see revisions like this sit for 2 years with nothing happening, I don't see anything wrong with this that would prevent it going in so I don't understand whats blocking it?,

if you had some tests and a release note I'd give it a LGTM (but as I've said before I'm not the code owner, but someone wanting to address defects and add capabilities. but I think we need to be able to move forward, people will object soon enough if they don't like it.)

Generally I don't understand why clang-format is so reluctant to change anything, As such we don't have many people involved and getting anything done (even defects) is extremely hard.

It looks like you met the criteria, and reviewers have been given ample opportunity to make an objection. the number of subscribers and like tokens would suggest this is wanted,

Please also add a line the in the release notes to say what you are adding. In the absence of any other constructive input all we can do is follow the guidance on doing a review, for what its worth I notice in the rest of LLVM there seems to be a much larger amount of commits that go in even without a review, I'm not sure what makes this area so strict, so reluctant to change especailly when revisions do seem to be reviewed.

I don't have any stake here, but i just want to point out that no tool (including clang-format)
will ever support all the things all the people out there will want it to support. But every
new knob is not just a single knob, it needs to work well with all the other existing knobs
(and all of the combination of knob params), and every new knob after that.

It's a snowball effect. Things can (and likely will, unless there is at least a *very* strict testing/quality policy
(which is https://clang.llvm.org/docs/ClangFormatStyleOptions.html#adding-additional-style-options about))
get out of hand real quickly..

Just 2c.

Correct we should always be adding tests and show we don't break existing tests... We need to apply the "Beyonce Rule".. "if you liked it you should have put a test on it."

We shouldn't just give up making improvements or fixing defects because something is hard or complex.

Adding the unit tests lets us see how this option will work in various cases, it will let us understand that its not breaking anything else.

I personally don't like to see revisions like this sit for 2 years with nothing happening, I don't see anything wrong with this that would prevent it going in so I don't understand whats blocking it?,

if you had some tests and a release note I'd give it a LGTM (but as I've said before I'm not the code owner, but someone wanting to address defects and add capabilities. but I think we need to be able to move forward, people will object soon enough if they don't like it.)

Generally I don't understand why clang-format is so reluctant to change anything, As such we don't have many people involved and getting anything done (even defects) is extremely hard.

It looks like you met the criteria, and reviewers have been given ample opportunity to make an objection. the number of subscribers and like tokens would suggest this is wanted,

Please also add a line the in the release notes to say what you are adding. In the absence of any other constructive input all we can do is follow the guidance on doing a review, for what its worth I notice in the rest of LLVM there seems to be a much larger amount of commits that go in even without a review, I'm not sure what makes this area so strict, so reluctant to change especailly when revisions do seem to be reviewed.

I don't have any stake here, but i just want to point out that no tool (including clang-format)
will ever support all the things all the people out there will want it to support. But every
new knob is not just a single knob, it needs to work well with all the other existing knobs
(and all of the combination of knob params), and every new knob after that.

It's a snowball effect. Things can (and likely will, unless there is at least a *very* strict testing/quality policy
(which is https://clang.llvm.org/docs/ClangFormatStyleOptions.html#adding-additional-style-options about))
get out of hand real quickly..

Just 2c.

Correct we should always be adding tests and show we don't break existing tests... We need to apply the "Beyonce Rule".. "if you liked it you should have put a test on it."

We shouldn't just give up making improvements or fixing defects because something is hard or complex.

The problem is that clang-format is already at a complexity level where most patches for new things we see are overly complex and will make adding new things *even harder*, reducing the ability to move at all.
As I've written before, I agree that we're in a bad state, and I'm truly sorry for it - we need to get active contributors into a state where they can make changes aligned with the architectural spirit of clang-format, which make clang-format at least not more complex :)

For this specific patch, yes, please change it as djasper suggested to be configurable in the BracketAlignmentStyle.

fdwr added a subscriber: fdwr.Mar 15 2019, 6:34 PM
fdwr awarded a token.Mar 15 2019, 6:37 PM
nyaapa added a subscriber: nyaapa.
saki7 added a subscriber: saki7.Apr 10 2019, 6:50 PM
msafi awarded a token.Apr 25 2019, 6:38 AM
msafi added a subscriber: msafi.
mnemotic added a subscriber: mnemotic.
mdrohmann added a subscriber: mdrohmann.

Will this option also work with dangling braces from initializers?

I have some code that looks like this:

return some_type_with_a_long_name{
    get_param_number_one(),
    get_param_number_two()
};

Clang format will put the brace at the end of the line:

return some_type_with_a_long_name{
    get_param_number_one(),
    get_param_number_two()};

I would like to keep the style of the first snippet.

quezak added a subscriber: quezak.
teohhanhui added a subscriber: teohhanhui.
aaptho added a subscriber: aaptho.Aug 30 2019, 3:26 PM

@stringham you seemingly have marked this revision so that no one else can edit it, so I cannot add it to the clang-format project or reassign reviewers, could you please change it to be public, it also means others cannot request changes or approve it (if they wanted to)

MyDeveloperDay added a project: Restricted Project.Sun, Oct 6, 1:30 PM

I'd like to assess where we are at with this revision, despite our concerns over additional complexity of clang-format, I don't think this is adding too much (I've seen far worse patches)

It appears to me that these changes are mainly in mustBreak,canBreak etc... however if you haven't added any unit tests I'm pretty sure things are going to go wrong when we run the other tests.

I'm not sure if you ever rebased with @mprobst changes or if those changes help you in any way

Given the level of likes of this feature, I think its worth at least looking at again, the quickness of your response yesterday suggests this might still be on your radar somewhere? it might be worth rebasing just to ensure it would go in cleanly, I could help by adding some unit tests if this is still of interest

MyDeveloperDay added a comment.EditedTue, Oct 8, 10:46 AM

I tried the patch and rebased it locally

(as there were conflicts with the current trunk)

The change caused other tests to fail (which doesn't completely surprise me)

most tests failures look associated with positioning of trailing brace for example

@@ -1,4 +1,4 @@
 void f() {
   bar([]() {} // Did not respect SpacesBeforeTrailingComments
-  );
+ );
 }

Below is a list of the tests that fail

[==========] 700 tests from 21 test cases ran. (6687 ms total)
[  PASSED  ] 692 tests.
[  FAILED  ] 8 tests, listed below:
[  FAILED  ] FormatTest.WrapsTemplateDeclarations
[  FAILED  ] FormatTest.BreaksStringLiteralsWithin_TMacro
[  FAILED  ] FormatTest.FormatsLambdas
[  FAILED  ] FormatTest.WrappedClosingParenthesisIndent
[  FAILED  ] FormatTestJS.FunctionParametersTrailingComma
[  FAILED  ] FormatTestJS.FunctionLiterals
[  FAILED  ] FormatTestJS.ArrowFunctions
[  FAILED  ] FormatTestJS.NestedLiterals

 8 FAILED TESTS
glfmn added a subscriber: glfmn.
njaldea added a subscriber: njaldea.