Page MenuHomePhabricator

clang-format: fix formatting of ObjC @synchronized blocks
ClosedPublic

Authored by Typz on Feb 9 2018, 1:13 AM.

Details

Summary

The blocks used to be formatted using the "default" behavior, and would
thus be mistaken for function calls followed by blocks: this could lead
to unexpected inlining of the block and extra line-break before the
opening brace.

They are now formatted similarly to @autoreleasepool blocks, as
expected:

@synchronized(self) {
    f();
}

Diff Detail

Repository
rL LLVM

Event Timeline

Typz created this revision.Feb 9 2018, 1:13 AM
Typz added inline comments.Feb 9 2018, 1:18 AM
lib/Format/UnwrappedLineParser.cpp
1130 ↗(On Diff #133570)

Wondering if formatting with this style is appropriate: the clang-format doc points in that direction, but it seems to me both @synchronized and @autoreleasepool are more akin to "control statements".

For consistency (esp. in ObjC++ code), as a user, I would tend to have these blocks indented like control statements while interfaces/implementations blocks would be indented like classes/structs.

So maybe it would be better to introduce a new BraceWrapping style 'AfterObjCSpecialBlock` to control these cases, matching the possibilities that are given for control statements vs classes. What do you think?

djasper edited reviewers, added: benhamilton, thakis; removed: djasper, klimek.Feb 9 2018, 4:58 AM
benhamilton added inline comments.Feb 9 2018, 7:46 AM
lib/Format/UnwrappedLineParser.cpp
1130 ↗(On Diff #133570)

Hmm, I definitely agree with that logic. I don't see them acting as declarations in any way, they are definitely like control statements.

Typz added inline comments.Feb 9 2018, 8:06 AM
lib/Format/UnwrappedLineParser.cpp
1130 ↗(On Diff #133570)

Ok, i can change this. Two questions though:

  • Should I do this in this patch, or a separate patch? (won't be a big change in any case, but it can still be seen as 2 separate issues/changes)
  • Should I introduce a new BraceWrapping flag (AfterObjCSpecialBlock), or simply apply AfterControlStatement to these blocks?
benhamilton added inline comments.Feb 12 2018, 8:29 AM
lib/Format/UnwrappedLineParser.cpp
1130 ↗(On Diff #133570)

Personally, I feel AfterControlStatement applies to these. I'm not an expert on this, though, so I will defer to others on the review.

Typz marked 4 inline comments as done.Feb 13 2018, 5:26 AM
Typz added inline comments.
lib/Format/UnwrappedLineParser.cpp
1130 ↗(On Diff #133570)

I created another diff to change the field that is used to control brace wrapping after these blocks: https://reviews.llvm.org/D43232

This patch here should now only relate to the parsing of @synchronized blocks.

benhamilton accepted this revision.Feb 15 2018, 8:19 AM

Just a question on the test.

unittests/Format/FormatTestObjC.cpp
193–198 ↗(On Diff #133570)

Why is the block repeated twice?

200–207 ↗(On Diff #133570)

Same question.

This revision is now accepted and ready to land.Feb 15 2018, 8:19 AM
Typz marked an inline comment as done.Feb 15 2018, 9:12 AM
Typz added inline comments.
unittests/Format/FormatTestObjC.cpp
193–198 ↗(On Diff #133570)

not sure why, I just copied the @autoreleasepool test code.
this may be to detect some more 'internal' error, which would lead to any visible effect on the first statement itself...

This revision was automatically updated to reflect the committed changes.