This is an archive of the discontinued LLVM Phabricator instance.

[clang-format] Fix ObjC style guesser to also iterate over child lines
ClosedPublic

Authored by benhamilton on Mar 22 2018, 10:21 AM.

Details

Summary

When I wrote ObjCHeaderStyleGuesser, I incorrectly assumed the
correct way to iterate over all tokens in AnnotatedLine was to
iterate over the linked list tokens starting with
AnnotatedLine::First.

However, AnnotatedLine also contains a vector
AnnotedLine::Children with child AnnotedLines which have their own
tokens which we need to iterate over.

Because I didn't iterate over the tokens in the children lines, the
ObjC style guesser would fail on syntax like:

#define FOO ({ NSString *s = ... })

as the statement(s) inside { ... } are child lines.

This fixes the bug and adds a test. I confirmed the test
failed before the fix, and passed after the fix.

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.Mar 22 2018, 10:21 AM
djasper accepted this revision.Mar 22 2018, 10:28 AM

Looks good, thank you!

lib/Format/Format.cpp
1517 ↗(On Diff #139464)

Maybe choose a name that indicates what the bool result value means, e.g. LinesContainObjCCode or something.

This revision is now accepted and ready to land.Mar 22 2018, 10:28 AM

CheckLineTokens -> LineContainsObjCCode

benhamilton marked an inline comment as done.Mar 22 2018, 10:33 AM
benhamilton added inline comments.
lib/Format/Format.cpp
1517 ↗(On Diff #139464)

Done, thanks!

This revision was automatically updated to reflect the committed changes.
benhamilton marked an inline comment as done.
djasper added inline comments.Mar 23 2018, 5:47 AM
cfe/trunk/lib/Format/Format.cpp
1544

Sorry that I missed this in the original review. But doesn't this still fail for Children of Child lines, etc.? I think this probably needs to be fully recursive.

benhamilton added inline comments.Mar 23 2018, 7:14 AM
cfe/trunk/lib/Format/Format.cpp
1544

Oh, of course. I'll send a follow-up.

benhamilton marked an inline comment as done.Mar 23 2018, 8:04 AM
benhamilton added inline comments.
cfe/trunk/lib/Format/Format.cpp
1544

Follow-up in D44831.