Page MenuHomePhabricator

Update block formatting to behave like Xcode
Needs ReviewPublic

Authored by KingOfBrian on Apr 23 2015, 6:44 PM.

Details

Reviewers
djasper
Summary

clang-format follows google block formatting style, which is very rare in the iOS world. I added a bool to make the block formatting behave more like Xcode.

Diff Detail

Event Timeline

KingOfBrian retitled this revision from to Update block formatting to behave like Xcode.
KingOfBrian updated this object.
KingOfBrian edited the test plan for this revision. (Show Details)
KingOfBrian added a reviewer: djasper.
KingOfBrian added a subscriber: Unknown Object (MLST).Apr 24 2015, 4:15 PM

Forgot to add cfe-commits.

djasper added inline comments.Apr 29 2015, 4:52 AM
include/clang/Format/Format.h
225

I this style documented somewhere? There are many corner cases that you aren't explicitly testing. E.g. what if there are non-block selectors in between? What if the selectors before the first block do not fit on one line?

ObjCXcodeBlockFormat is not a good name. It would be good to have something that actually describes the behavior. Like this it is not really discoverable. And this does not really change the behavior of blocks in general, but the behavior of blocks in method exprs, IIUC.

Also, the comment needs to be quite a bit more elaborate.

lib/Format/ContinuationIndenter.cpp
419

Either remove the comment again or make it more elaborate. As is, it seems more confusing.

857

!Style.ObjCXcodeBlockFormat

unittests/Format/FormatTest.cpp
9803

Again, is there some documentation of what "Apple style" is?

9804

It seems quite weird that this style would only affect ColumnLimit 0. If you'd want to add it, we need to make it work for styles with column limits, too.

9818–9821

Whether Xcode does this or not, my personal opinion is that this is quite terrible. The alignment of a few colons that are many lines a part is next to useless where as this looks very messy, especially if the difference between block selector names is more than 1 character.

9861

What is this testing that's not already tested above?

KingOfBrian added inline comments.Apr 29 2015, 9:44 AM
include/clang/Format/Format.h
225

I haven't been able to find it documented anywhere, I looked around for a while. But it's been the same formatting in Xcode since blocks were introduced. I'll add some more tests for long selector-parts and see how it behaves.

I'm a bit stumped on the naming for behavior so I opted to name it based on the origin -- similar in approach to allman vs soustroup. I'm not sure there's a name that is discoverable, since the difference between this and the current google style is pretty nuanced. Would changing it to a style enumeration be more acceptable?

I added the prefix ObjC to imply that this only affects the behavior of blocks in method exprs, obviously that was not clear enough.

Also, reading the comment again, it's wrong, sorry about that. The comment explains the inverse of one of the two behavior changes - I'll take another stab at it, but without a good name for the flag, I'm a bit stumped. Let me think about it.

unittests/Format/FormatTest.cpp
9804

Right, let me look into that.

9818–9821

I think Apple developers may have a degree of Stockholm syndrome here, but it is a style that people want at this point. I dislike it personally too, and usually don't do line breaks so it left aligns. If I do, I keep the blocks very short, or break them out into a variable.

Multi-block obj-c can get ugly no matter what way you cut it. For instance, I thought the multi-block google style was so terrible that it was a bug!

9861

Good point, I'll update it.

djasper added inline comments.May 4 2015, 4:10 AM
include/clang/Format/Format.h
225

The fundamental question is, whether we want clang-format to support this style. I don't think it is useful to support it, just because it is what some other tool creates. That would mean that clang-format would have to implement all the flaws of other indentation tools, which we certainly do not want to do.

That being said, if this is widely used and ideally documented somewhere, it might be worth adding. But we certainly need to define it precisely in the comment (especially is there is no other documentation) and we need to make it work with a non-zero column limit.

KingOfBrian added inline comments.May 5 2015, 10:59 AM
include/clang/Format/Format.h
225

After thinking about this some, this does 2 separate style modifications, and if the goal is a discoverable config format, I these two things could be reflected in 2 different configuration options.

The first change is to not do left compression suggested by the google style. Also note that in the google style it states 'inlined blocks may have their segments left-aligned ... This helps when invocations contain multiple inlined blocks'. All this change does is provide the option to not perform this special case formatting (which is allowed by the style guide). What do you think about this being reflected via a 'ObjcLeftAlignMultipleBlocks` option?

The second change is to not force line breaks on multiple-argument ObjCMethodExpr. This only works with zero column limit, I have to hunt down the other code path for a column limit.

Your argument of not re-creating bad styles makes a lot of sense to me. I want to make sure that we agree that the style in the test on line 9855 is worth re-creating. This format is used a lot, although it's not explicitly documented anywhere that I can tell. The other styles are incidental to getting that working, although they have the bonus of matching the current Xcode tool-chain. I'm not going to be disappointed if the other formats don't work, but it's going to be very hard to get adoption in the iOS world if the styles I mentioned above aren't re-creatable. (also -- going to comment on them explicitely incase numbers change)

unittests/Format/FormatTest.cpp
9860

The style above is the block format I'm looking to re-create.

KingOfBrian updated this revision to Diff 24971.May 5 2015, 1:39 PM

Here's an update that does two things

  1. Splits the behavior changes into 2 bools, since they were controlling distinct formatting behaviors
  2. Adds support for non-zero column width for ObjCAvoidLineBreaksForInlineBlocks. Currently there's potentially broken behavior, where the decision to split an objective-c line is determined by the length of the opening [ to the close ]. In the case of in-line blocks, that calculation is deceptive as it includes blocks, which break the line. I added getLengthToLineTerminator that matches the closing ] or the first token with children lines. I only changed this behavior when ObjCAvoidLineBreaksForInlineBlocks is enabled, but it could be argued that this should be default behavior. I opted for compatibility unless you think otherwise.

This looks great, Brian!

Why is formatting within blocks/closures different to what I define for code that's not inside a block?
It really threw me when code within blocks started being wrapped when I'd already set a ColumnLimit of 0.

I'm quite keen to see this patch merged, if only to get familiar formatting back to my blocks in Xcode projects.

@djasper so was the decision here to not merge these options?

I think it's fair that you require a documented style guide somewhere (which Apple don't currently publish), however it would be good to at least have generic options in clang-format that allow us to replicate the unpublished style, even without official support in the tool.

What do we need to do to get support for options like ObjCAvoidLineBreaksForInlineBlocks and ObjCLeftAlignMultipleBlocks into clang-format? There are potentially one or two more options around dictionary/array literal alignment/newlines that I'd like to add — but I'm not going to spend any time drawing up PRs if there's no way they'll ever be merged.