This is an archive of the discontinued LLVM Phabricator instance.

[clang-format] Don't detect call to ObjC class method as C++11 attribute specifier
ClosedPublic

Authored by rkgibson2 on Jul 12 2019, 7:00 AM.

Details

Summary

Previously, clang-format detected something like the following as a C++11 attribute specifier.

@[[NSArray class]]

instead of an array with an Objective-C method call inside. In general, when the attribute specifier checking runs, if it sees 2 identifiers in a row, it decides that the square brackets represent an Objective-C method call. However, here, class is tokenized as a keyword instead of an identifier, so this check fails.

To fix this, the attribute specifier first checks whether the first square bracket has an "@" before it. If it does, then that square bracket is not the start of a attribute specifier because it is an Objective-C array literal. (The assumption is that @[[.*]] is not valid C/C++.)

Diff Detail

Repository
rL LLVM

Event Timeline

rkgibson2 created this revision.Jul 12 2019, 7:00 AM
Herald added a project: Restricted Project. · View Herald TranscriptJul 12 2019, 7:00 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript
rkgibson2 edited the summary of this revision. (Show Details)Jul 12 2019, 7:08 AM
rkgibson2 added a reviewer: benhamilton.

Thanks for the fix. One question: how does the real Clang parser deal with this case? Is it something that's actually ambiguous in the ObjC++ grammar, I wonder?

benhamilton added inline comments.Jul 12 2019, 8:19 AM
clang/lib/Format/TokenAnnotator.cpp
400–413 ↗(On Diff #209477)

Maybe we should check the token before AttrTok to see if it's tok::at, rather than checking for an identifier followed by tok::kw_class?

I don't think there's any valid C++11 attribute specifier sequence of @[[]].

benhamilton requested changes to this revision.Jul 12 2019, 8:20 AM
This revision now requires changes to proceed.Jul 12 2019, 8:20 AM
rkgibson2 marked an inline comment as done.Jul 12 2019, 8:40 AM

Thanks for the fix. One question: how does the real Clang parser deal with this case? Is it something that's actually ambiguous in the ObjC++ grammar, I wonder?

I am not sure about real Clang. I actually don't know much about how it works. I just ran into this bug and wanted to see if it was fixable. My guess is the @ sign marks the bracket as an array literal. In clang-format, isCpp11AttributeSpecifier is checked before checking whether the previous token is @.

clang/lib/Format/TokenAnnotator.cpp
400–413 ↗(On Diff #209477)

Ok, sure. I considered that at first but I'm not familiar with C++11 enough to say. I thought it wasn't valid, but I wasn't sure.

rkgibson2 updated this revision to Diff 209496.Jul 12 2019, 8:41 AM

Respond to comments and change detection method

aaron.ballman added inline comments.
clang/lib/Format/TokenAnnotator.cpp
389 ↗(On Diff #209496)

Clang has a feature flag to enable support for double-square bracket attributes in more than just C++ mode, and this is enabled by default in C2x mode. This check for isCpp() makes me suspect we may be doing the wrong thing here.

rkgibson2 updated this revision to Diff 209504.Jul 12 2019, 9:00 AM

Update comment

This revision is now accepted and ready to land.Jul 12 2019, 10:41 AM
benhamilton added inline comments.Jul 12 2019, 10:42 AM
clang/unittests/Format/FormatTest.cpp
7030–7032 ↗(On Diff #209504)

Consider adding another test for a method besides +class.

rkgibson2 updated this revision to Diff 209777.Jul 15 2019, 1:50 AM

Add more tests

rkgibson2 edited the summary of this revision. (Show Details)Jul 15 2019, 1:54 AM

I don't think I have commit access. Could someone land this for me?

This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptJul 16 2019, 2:29 PM

Submitted as r366267. Thanks!

clang/lib/Format/TokenAnnotator.cpp
389 ↗(On Diff #209496)

Good point. I filed https://bugs.llvm.org/show_bug.cgi?id=42645 to revisit this.