This is an archive of the discontinued LLVM Phabricator instance.

[clang-format] Ensure ObjC selectors with 0 args are annotated correctly
ClosedPublic

Authored by benhamilton on Mar 28 2018, 1:11 PM.

Details

Summary

Previously, clang-format would incorrectly annotate 0-argument
Objective-C selector names as TT_TrailingAnnotation:

% echo "-(void)foo;" > /tmp/test.m
% ./bin/clang-format -debug /tmp/test.m
Language: Objective-C
----
Line(0, FSC=0): minus[T=68, OC=0] l_paren[T=68, OC=1] void[T=68, OC=2]
r_paren[T=68, OC=6] identifier[T=68, OC=7] semi[T=68, OC=10]
Line(0, FSC=0): eof[T=68, OC=0]
Run 0...
AnnotatedTokens(L=0):
 M=0 C=0 T=ObjCMethodSpecifier S=1 B=0 BK=0 P=0 Name=minus L=1 PPK=2
 FakeLParens= FakeRParens=0 Text='-'
 M=0 C=1 T=Unknown S=1 B=0 BK=0 P=33 Name=l_paren L=3 PPK=2
 FakeLParens= FakeRParens=0 Text='('
 M=0 C=1 T=Unknown S=0 B=0 BK=0 P=140 Name=void L=7 PPK=2 FakeLParens=
 FakeRParens=0 Text='void'
 M=0 C=0 T=CastRParen S=0 B=0 BK=0 P=43 Name=r_paren L=8 PPK=2
 FakeLParens= FakeRParens=0 Text=')'
 M=0 C=1 T=TrailingAnnotation S=0 B=0 BK=0 P=120 Name=identifier L=11
 PPK=2 FakeLParens= FakeRParens=0 Text='foo'
 M=0 C=0 T=Unknown S=0 B=0 BK=0 P=23 Name=semi L=12 PPK=2 FakeLParens=
 FakeRParens=0 Text=';'

This caused us to incorrectly indent 0-argument wrapped selectors
when Style.IndentWrappedFunctionNames was false, as we thought
the 0-argument ObjC selector name was actually a trailing
annotation (which is always indented).

This diff fixes the issue and adds tests.

Test Plan: New tests added. Confirmed tests failed before diff.

After diff, tests passed. Ran tests with:
% make -j12 FormatTests &&
./tools/clang/unittests/Format/FormatTests

Diff Detail

Repository
rC Clang

Event Timeline

benhamilton created this revision.Mar 28 2018, 1:11 PM
jolesiak added inline comments.Mar 29 2018, 12:59 AM
unittests/Format/FormatTestObjC.cpp
528

I know that Style.IndentWrappedFunctionNames is false by default, but how about adding

Style.IndentWrappedFunctionNames = false;

before these verifyFormat calls? I feel like that makes it more readable and makes test independent on this Style constant.

jolesiak added inline comments.Mar 29 2018, 1:20 AM
unittests/Format/FormatTestObjC.cpp
528

This comment is kind of irrelevant as this change is included in D45005.

jolesiak added inline comments.Mar 29 2018, 1:30 AM
unittests/Format/FormatTestObjC.cpp
528

My bad, this change is NOT included in D45005. (I don't know how to remove inline comment)

benhamilton marked 3 inline comments as done.Mar 29 2018, 8:18 AM
benhamilton added inline comments.
unittests/Format/FormatTestObjC.cpp
528

Sure, done.

djasper added inline comments.Mar 30 2018, 8:02 AM
lib/Format/TokenAnnotator.cpp
1350

Isn't it wrong that we detect this as a cast r_paren in the first place?

benhamilton added inline comments.Mar 30 2018, 8:06 AM
lib/Format/TokenAnnotator.cpp
1350

Fantastic question, I asked myself the same thing.

I tried a few variations on this (leaving it as TT_Unknown, making a new type, etc.) and discovered there is at least one existing place which relies on the TT_CastRParen type as an indicator of ObjC code. Example:

https://github.com/llvm-mirror/clang/blob/e37a191e99773959118155304ec2ed0bc0d591c2/lib/Format/TokenAnnotator.cpp#L394

I can fix those, but if I do so, I think it should be a separate diff. What do you think?

benhamilton marked 2 inline comments as done.Apr 2 2018, 9:39 AM
benhamilton added inline comments.
lib/Format/TokenAnnotator.cpp
1350

@djasper and I talked about this on Friday and agreed we should follow up separately.

I filed https://bugs.llvm.org/show_bug.cgi?id=36976 to follow up.

benhamilton marked an inline comment as done.Apr 4 2018, 8:33 AM

Any more comments on this one, folks? I'd love to land this fix.

jolesiak accepted this revision.Apr 4 2018, 8:46 AM

lgtm

This revision is now accepted and ready to land.Apr 4 2018, 8:46 AM
djasper accepted this revision.Apr 5 2018, 2:37 AM
djasper added inline comments.
lib/Format/TokenAnnotator.cpp
1350

You might also want to add a

// FIXME: ...

Add FIXME comments.

This revision was automatically updated to reflect the committed changes.