Page MenuHomePhabricator

[clang-format] Improve detection of ObjC for-in statements

Authored by benhamilton on Feb 28 2018, 1:40 PM.



Previously, clang-format would detect the following as an
Objective-C for-in statement:

for (int x = in.value(); ...) {}

because the logic only decided a for-loop was definitely *not*
an Objective-C for-in loop after it saw a semicolon or a colon.

To fix this, I delayed the decision of whether this was a for-in
statement until after we found the matching right-paren, at which
point we know if we've seen a semicolon or not.

Test Plan: New tests added. Ran tests with:

make -j12 FormatTests && ./tools/clang/unittests/Format/FormatTests

Diff Detail


Event Timeline

benhamilton created this revision.Feb 28 2018, 1:40 PM
jolesiak accepted this revision.Mar 5 2018, 8:43 AM
This revision is now accepted and ready to land.Mar 5 2018, 8:43 AM
djasper added inline comments.
288 ↗(On Diff #136386)

There can be only one ObjCForIn token in any for loop, right? If that's the case, can we just remember that in addition to (or instead of) MightBeObjCForRangeLoop? That way, we wouldn't have to re-iterate over all the tokens here, but could just set this directly.

krasimir added inline comments.Mar 5 2018, 10:48 AM
12002 ↗(On Diff #136386)

Please also add this instances as formatting tests.

benhamilton marked an inline comment as done.

One more cleanup

288 ↗(On Diff #136386)

Nice optimization, done.

12002 ↗(On Diff #136386)

Thanks, tests added.

The new formatting tests revealed a new regression I'd introduced: we were not inserting a space between keyword 'in' and the opening [ of the ObjC message send.

This was because the ObjC message send parsing logic depended on the TT_ObjCForIn keyword being set before parsing the [, but by delaying setting the type until after parsing the for-loop was complete, we broke that logic.

Fixed regression and made sure tests passed.

krasimir added inline comments.Mar 6 2018, 5:23 AM
778 ↗(On Diff #137093)

Please move the ObjC-specific instances to FormatTestObjC.cpp.

benhamilton marked an inline comment as done.
  • Move ObjC-specific tests to FormatTestObjC.cpp.
778 ↗(On Diff #137093)


This revision was automatically updated to reflect the committed changes.