Page MenuHomePhabricator

[clang-format] Add option for not breaking line before ObjC params
ClosedPublic

Authored by ghvg1313 on Dec 2 2019, 2:18 PM.

Details

Summary

From clang-format version 3.7.0 and up, , there is no way to keep following format of ObjectiveC block:

- (void)_aMethod
{
    [self.test1 t:self w:self callback:^(typeof(self) self, NSNumber *u, NSNumber *v) {
        u = c;
    }]
}

Regardless of the change in .clang-format configuration file, all parameters will be lined up so that colons will be on the same column, like following:

- (void)_aMethod
{
    [self.test1 t:self
                w:self
         callback:^(typeof(self) self, NSNumber *u, NSNumber *v) {
             u = c;
         }]
}

Considering with ObjectiveC, the first code style is cleaner & more readable for some people, I've added a config option: ObjCDontBreakBeforeNestedBlockParam (boolean) so that if it is enable, the first code style will be favored.

Diff Detail

Event Timeline

ghvg1313 created this revision.Dec 2 2019, 2:18 PM
Herald added a project: Restricted Project. · View Herald TranscriptDec 2 2019, 2:18 PM
Herald added a subscriber: cfe-commits. · View Herald Transcript
ghvg1313 removed a reviewer: klimek.Dec 2 2019, 2:23 PM
ghvg1313 retitled this revision from Add option for not breaking line before ObjC params to [clang-format] Add option for not breaking line before ObjC params.
ghvg1313 added reviewers: MyDeveloperDay, gribozavr.

Could you update the ClangStyleOption.rst (using the tool in docs/tools)

and as you are adding a new option would you add a line or two in the docs/ReleaseNotes.rst in the clang-format section

ghvg1313 updated this revision to Diff 231790.Dec 2 2019, 3:29 PM
  • Update documents
MyDeveloperDay requested changes to this revision.Dec 2 2019, 11:55 PM

I can tell from https://zed0.co.uk/clang-format-configurator/ that it's not possible to get it to the form you suggested, I'm not opposed to accepting this, I'm just not an ObjectiveC person so I haven't used clang-format on code like this, I just don't want to break others

requesting changes due to removed line?

clang/lib/Format/ContinuationIndenter.cpp
871

I'm a little confused by the code, this feels to me like its covering something more than a specific than just the ObjectiveC case, what is meant here by a nestedblockspecialcase? this feels like we are turning off BreakBeforeParameter by another method?

I get the code below, just wondering why this is also needed.

clang/lib/Format/Format.cpp
809

did you mean to remove this line?

This revision now requires changes to proceed.Dec 2 2019, 11:55 PM
gribozavr2 added inline comments.
clang/include/clang/Format/Format.h
1693

I don't know much about ClangFormat, but I'd prefer to keep the option name positive ("ObjCBreakBefore...").

MyDeveloperDay added inline comments.Dec 3 2019, 4:22 AM
clang/include/clang/Format/Format.h
1693

+1 that

ghvg1313 marked an inline comment as done.Dec 3 2019, 1:16 PM
ghvg1313 added inline comments.
clang/lib/Format/ContinuationIndenter.cpp
871

I think you are right, bin packing is already avoided as the comment says so I don't have to do anything extra.

Was trying to handle linebreaking within block as well. Not needed indeed.

ghvg1313 updated this revision to Diff 231971.Dec 3 2019, 1:16 PM
ghvg1313 marked an inline comment as done.
  • rename option, remove handlers for nested block itself

so now I think this is better because it's encased inside your TT_ObjCMethodExpr method which means it should only impact ObjectiveC

Nit on the documentation
Nit missing parse tests

but otherwise you are pretty close, make those changes and I'll mark accepted

clang/docs/ReleaseNotes.rst
159

Nit: this section should really go into the Format.h and the ClangFormatStyle.rst, for the release note a couple of lines is probably ample.

But ultimately this example will disappear from the release notes (as they move on) and I think this example is good to have in the main documentation.

clang/unittests/Format/FormatTest.cpp
14446 ↗(On Diff #231971)

Nit: you are missing a CHECK_PARSE_BOOL tests

MyDeveloperDay requested changes to this revision.Dec 4 2019, 12:55 AM
This revision now requires changes to proceed.Dec 4 2019, 12:55 AM
ghvg1313 updated this revision to Diff 232177.Dec 4 2019, 10:28 AM
ghvg1313 marked an inline comment as done.
  • Move documents from release notes to header and proper rst, register test
MyDeveloperDay accepted this revision.Dec 4 2019, 11:23 AM

LGTM

A little advice as you fix each comment check the "Done" button.

I notice you are a new user, will you need help landing it?

This revision is now accepted and ready to land.Dec 4 2019, 11:23 AM

LGTM

A little advice as you fix each comment check the "Done" button.

I notice you are a new user, will you need help landing it?

Thanks for the review!
And yes I do need help landing it, quite new to the community, not sure if I should head to github and dupe the changes or something else...

ghvg1313 marked 5 inline comments as done.Dec 4 2019, 11:31 AM

@MyDeveloperDay Anything I'll need to do on my end to start the landing process?

MyDeveloperDay requested changes to this revision.Dec 6 2019, 9:09 AM
MyDeveloperDay added inline comments.
clang/unittests/Format/FormatTest.cpp
14468 ↗(On Diff #232177)

This line does not compile, I think whilst we are on the subject it would be better to move this test into

FormatTestObjC.cpp

This revision now requires changes to proceed.Dec 6 2019, 9:09 AM
ghvg1313 updated this revision to Diff 234621.Dec 18 2019, 2:27 PM
  • Handle parameter break between blocks
  • Fix test cases and move them to objcTest
  • rebase
ghvg1313 marked 2 inline comments as done.Dec 18 2019, 2:35 PM
ghvg1313 added inline comments.
clang/lib/Format/ContinuationIndenter.cpp
871

Notice I get this back again, it's actually to avoid parameter break in between blocks, so we don't get inconsistent parameter break between blocks, for example:

- (void)_aMethod
{
    [self.test1 t:self
                    w:self
         callback:^(typeof(self) self, NSNumber *u, NSNumber *v) {
             u = c;
         } x:self callback2:^(typeof(self) self, NSNumber *u, NSNumber *v) {
             u = c;
         }]
}

Originally, the second block will trigger the parameter break in between itself and the previous block.

MyDeveloperDay accepted this revision.Dec 19 2019, 1:17 AM

Thanks for the patch.

This revision is now accepted and ready to land.Dec 19 2019, 1:17 AM

Thanks for the patch.

Happy new year!
Could you help landing this diff? I don't know exactly what to do from here

@MyDeveloperDay gentle ping for help merging

MyDeveloperDay requested changes to this revision.Jan 19 2020, 7:59 AM

I need you to rebase this fix, also you need to clang-format it as well. Format.h/ContinuationIndenter.cpp and FormatTestObjC.cpp all failed clang-format check

This revision now requires changes to proceed.Jan 19 2020, 7:59 AM
ghvg1313 updated this revision to Diff 239458.Jan 21 2020, 4:41 PM
  • rebase master
  • formatting code
clang/docs/ReleaseNotes.rst
164

is this line part of this patch?

ghvg1313 marked 2 inline comments as done.Jan 24 2020, 11:37 AM
ghvg1313 added inline comments.
clang/docs/ReleaseNotes.rst
164

No, rebased again and got rid of it

ghvg1313 updated this revision to Diff 240260.Jan 24 2020, 11:37 AM
ghvg1313 marked an inline comment as done.
  • rebase master
MyDeveloperDay accepted this revision.Jan 27 2020, 6:45 AM
MyDeveloperDay added a project: Restricted Project.
This revision is now accepted and ready to land.Jan 27 2020, 6:46 AM
This revision was automatically updated to reflect the committed changes.