Page MenuHomePhabricator

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

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

Details

Summary

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

Repository
rL LLVM

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.
lib/Format/TokenAnnotator.cpp
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
unittests/Format/FormatTest.cpp
12002 ↗(On Diff #136386)

Please also add this instances as formatting tests.

benhamilton marked an inline comment as done.

One more cleanup

lib/Format/TokenAnnotator.cpp
288 ↗(On Diff #136386)

Nice optimization, done.

unittests/Format/FormatTest.cpp
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
unittests/Format/FormatTest.cpp
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.
unittests/Format/FormatTest.cpp
778 ↗(On Diff #137093)

Done.

This revision was automatically updated to reflect the committed changes.