Page MenuHomePhabricator

[clang-format] Proposal for changes to Objective-C block formatting
Needs ReviewPublic

Authored by ksuther on Feb 28 2016, 5:27 PM.

Details

Reviewers
djasper
Summary

Changes to clang-format's Objective-C block formatting over the past year have made clang-format's output deviate from what is expected (in my opinion).

There are three changes in particular:

  • Method calls with multiple block parameters has each block indented further in
  • Nested blocks get indented one level in
  • Method calls that end with a block parameter always get forced to a new line and indented

The end of this message has examples of formatting with and without these changes.

This patch undoes one revision which causes methods with multiple block parameters to indent for each parameter (r236598) and adds two new options.
AllowNewlineBeforeBlockParameter makes the change in r235580 optional.
IndentNestedBlocks makes the change in r234304 optional.

Some relevant Bugzilla issues:
https://llvm.org/bugs/show_bug.cgi?id=23585
https://llvm.org/bugs/show_bug.cgi?id=23317

This patch came out of a discussion here https://github.com/square/spacecommander/issues/33, where we're using a custom version of clang-format with these options added. Based on the Bugzilla issues, it seems that we're not alone.

Thanks!


Current trunk:

bin/clang-format -style="{BasedOnStyle: WebKit, ColumnLimit: 0, IndentWidth: 4}" ~/clang-format.m

- (void)test
{
    [self block:^(void) {
        doStuff();
    }
        completionHandler:^(void) {
            doStuff();

            [self block:^(void) {
                doStuff();
            }
                completionHandler:^(void) {
                    doStuff();
                }];
        }];

    [[SessionService sharedService] loadWindow:aWindow
                               completionBlock:^(SessionWindow* window) {
                                   if (window) {
                                       [self doStuff];
                                   }
                               }];
}

With this patch applied:

bin/clang-format -style="{BasedOnStyle: WebKit, ColumnLimit: 0, IndentWidth: 4, IndentNestedBlocks: false, AllowNewlineBeforeBlockParameter: false}"~/clang-format.m

- (void)test
{
    [self block:^(void) {
        doStuff();
    } completionHandler:^(void) {
        doStuff();

        [self block:^(void) {
            doStuff();
        } completionHandler:^(void) {
            doStuff();
        }];
    }];

    [[SessionService sharedService] loadWindow:aWindow completionBlock:^(SessionWindow* window) {
        if (window) {
            [self doStuff];
        }
    }];
}

Diff Detail

Event Timeline

ksuther updated this revision to Diff 49332.Feb 28 2016, 5:27 PM
ksuther retitled this revision from to [clang-format] Proposal for changes to Objective-C block formatting.
ksuther updated this object.
ksuther added a reviewer: djasper.
ksuther added a subscriber: cfe-commits.

Aside from the lack of tests, and AllowNewlineBeforeBlockParameter being outside of BraceWrapping, this is good! Much closer to how Apple's documentation and Xcode format blocks! ๐Ÿ‘

include/clang/Format/Format.h
421

Perhaps this would be better placed within the custom BraceWrapping options? Something simpler like BeforeBlockParameter.

tonyarnold added a comment.EditedFeb 28 2016, 6:48 PM

I did find one bug though โ€” if there's any parameters in between the two blocks, they'll end up forcing a newline:

@implementation SomeClass

- (void)test
{
        [sharedController passingTest:^BOOL(id application) {
            return application.thing;
        } withTimeout:kTimeout completionHandler:^(BOOL success, NSError *error) {
            if (success == NO && error != nil)
            {
              // Do the thing
            }
        }];
}

@end

Wrongly becomes:

@implementation SomeClass

- (void)test
{
    [sharedController passingTest:^BOOL(id application) {
        return application.thing;
    } withTimeout:kTimeout
        completionHandler:^(BOOL success, NSError *error) {
            if (success == NO && error != nil)
            {
                // Do the thing
            }
        }];
}

@end
ksuther updated this revision to Diff 49928.Mar 6 2016, 8:03 PM

Thanks for the comments. I've made some changes that eliminates reverting r236598 and instead makes the behavior part of IndentNestedBlocks. That allows the Google Obj-C code style (https://google.github.io/styleguide/objcguide.xml#Blocks) to still work by default. The issue with a parameter between block parameters has also been fixed (as part of AllowNewlineBeforeBlockParameter).

Apologies if I'm missing something obvious, but I don't see how AllowNewlineBeforeBlockParameter belongs in BraceWrapping. Blocks aren't the same as braces, in the original example, this option would be controlling the newline insertion for completionBlock:^(SessionWindow* window) { and not just the opening brace.

Blocks aren't the same as braces

Yeah, good point. I guess I was just seeing the brace in the block, but that's the wrong way to think about it.

k06a awarded a token.Mar 25 2016, 12:19 AM

Was this lost in the woods? I believe this is a valid change when working in Objective-C

It seems to have been. I've been using a modified version of clang-format with this change applied (as well as http://reviews.llvm.org/D17922) since I submitted this.

Myrronth added a subscriber: Myrronth.

Do you mind sharing your modified version? The fork in the discussion you mentioned (https://github.com/square/spacecommander/issues/33) seems to be out-of-date or at least does not have the AllowNewlineBeforeBlockParameter setting.

My fork of spacecommander has a version of clang-format with the option. It's a couple of years old at this point, but it has been running without any issues.

Thanks, Iโ€™ll give it a try.