Page MenuHomePhabricator

[clang-format/ObjC] Correctly parse Objective-C methods with 'class' in name
ClosedPublic

Authored by benhamilton on May 18 2018, 3:57 PM.

Details

Summary

Please take a close look at this CL. I haven't touched much of
UnwrappedLineParser before, so I may have gotten things wrong.

Previously, clang-format would incorrectly format the following:

@implementation Foo

- (Class)class {
}

- (void)foo {
}

@end

as:

@implementation Foo

- (Class)class {
}

    - (void)foo {
}

@end

The problem is whenever UnwrappedLineParser::parseStructuralElement()
sees any of the keywords class, struct, or enum, it calls
parseRecord() to parse them as a C/C++ record.

This causes subsequent lines to be parsed incorrectly, which
causes them to be indented incorrectly.

In Objective-C/Objective-C++, these keywords are valid selector
components.

This diff fixes the issue by explicitly handling + and - lines
inside @implementation / @interface / @protocol blocks
and parsing them as Objective-C methods.

Test Plan: New tests added. Ran tests with:

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

Diff Detail

Repository
rC Clang

Event Timeline

benhamilton created this revision.May 18 2018, 3:57 PM
djasper edited reviewers, added: klimek; removed: djasper.May 20 2018, 11:21 PM
jolesiak added inline comments.May 23 2018, 4:42 AM
lib/Format/UnwrappedLineParser.cpp
2137

tok::r_brace could be skiped - see comment to line 2143.

2143

We have to add return after addUnwrappedLine as parseBlock does consume tok::r_brace. Without return we will consume tokens after }. This problem will rarely occur as most lines end with tok::semi or tok::r_brace and it will be terminated properly (however maybe not handled properly as we just skip every token in else) by if branch.

Test like:

@implementation Foo
- (foo)foo {
}
@end
@implementation Bar
- (bar)bar {
}
@end

will distinguish version with return from one without. Therefore, I think we should add it.

benhamilton marked 2 inline comments as done.
lib/Format/UnwrappedLineParser.cpp
2137

Done.

2143

Done, test added.

LGTM, would be nice though if somebody else took a look (@klimek ?).

jolesiak accepted this revision.May 24 2018, 12:48 AM
This revision is now accepted and ready to land.May 24 2018, 12:48 AM
This revision was automatically updated to reflect the committed changes.