Page MenuHomePhabricator

[clang-format] fix regression recognizing casts in Obj-C calls
ClosedPublic

Authored by krasimir on Oct 18 2019, 5:51 AM.

Details

Summary

r373922 added checks for a few tokens that, following an ) make it
unlikely that the ) is the closing paren of a cast expression. The
specific check for tok::l_square there introduced a regression for
casts of Obj-C calls, like:

(cast)[func arg]

From the tests added in r373922, I believe the tok::l_square case is added to
capture the case where a non-cast ) is directly followed by an
attribute specifier, like:

int f(int x) [[noreturn]];

I've specialized the code to look for such attribute specifier instead
of tok::l_square in general. Also, I added a regression test and moved
the test cases added in r373922 to an already existing place documenting
other instances of historically misidentified casts.

Diff Detail

Event Timeline

krasimir created this revision.Oct 18 2019, 5:51 AM
Herald added a project: Restricted Project. · View Herald TranscriptOct 18 2019, 5:51 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript
krasimir edited the summary of this revision. (Show Details)Oct 18 2019, 5:52 AM
krasimir added a reviewer: MyDeveloperDay.

LGTM, thanks for the patch

MyDeveloperDay accepted this revision.Oct 18 2019, 6:23 AM
This revision is now accepted and ready to land.Oct 18 2019, 6:23 AM
rdwampler added inline comments.
lib/Format/TokenAnnotator.cpp
1612 ↗(On Diff #225604)

I think the issue r373922 was fixing is only related to the delete. Can this check be move further up where we explicitly check if the token is the delete keyword?

This revision was automatically updated to reflect the committed changes.
krasimir marked an inline comment as done.Oct 18 2019, 11:29 AM
krasimir added inline comments.
lib/Format/TokenAnnotator.cpp
1612 ↗(On Diff #225604)

Sorry, I didn't notice this comment until now. I can investigate whether all the other cases for non-delete methods are already covered, it would be interesting. + @MyDeveloperDay, who might have better insights into this.

rC373922: [clang-format] [PR27004] omits leading space for noexcept when formatting… was to fix https://bugs.llvm.org/show_bug.cgi?id=27004 which wasn't just related to delete it occurred in other cases. (operator++) and there could be other cases i guess.

I think the check for delete above the code that was added was checking for delete being on the left i.e. delete() and delete(x) is not a cast (sorry I wasn't the author for that part, so might not be correct)

However, I appreciate this patch (or any patch for that matter) that strengthens our tests in this way.