Page MenuHomePhabricator

add new check for property declaration
ClosedPublic

Authored by Wizard on Nov 8 2017, 8:20 PM.

Details

Summary

This check finds property declarations in Objective-C files that do not follow the pattern of property names in Apple's programming guide. The property name should be in the format of Lower Camel Case or with some particular acronyms as prefix.

Example:
@property(nonatomic, assign) int lowerCamelCase;

@property(nonatomic, strong) NSString *URLString;

Test plan: ninja check-clang-tools

Diff Detail

Repository
rL LLVM

Event Timeline

Wizard created this revision.Nov 8 2017, 8:20 PM
Wizard edited the summary of this revision. (Show Details)Nov 8 2017, 8:25 PM
Wizard added reviewers: benhamilton, hokein.
hokein edited edge metadata.Nov 9 2017, 2:58 AM
hokein added a subscriber: cfe-commits.

Next time, please remember to add cfe-commits to the subscriber.

clang-tidy/objc/PropertyDeclarationCheck.cpp
14 ↗(On Diff #122197)

Do we need this header?

25 ↗(On Diff #122197)

I might miss some background context.

The fix of the check seems to me that it does more things it should. It removes all the non-alphabetical prefix characters, I'd be conservative of the fix here (just fix the case "CamelCase", and only give a warning for other cases).

docs/clang-tidy/checks/objc-property-declaration.rst
7 ↗(On Diff #122197)

Google style? but the link you provided is Apple.

27 ↗(On Diff #122197)

nit: add a newline.

benhamilton added inline comments.Nov 9 2017, 8:40 AM
docs/clang-tidy/checks/objc-property-declaration.rst
7 ↗(On Diff #122197)

Google's Objective-C style guide is a list of additions on top of Apple's Objective-C style guide.

Property naming standards are defined in Apple's style guide, and not changed by Google's.

benhamilton added inline comments.Nov 9 2017, 8:42 AM
clang-tidy/objc/PropertyDeclarationCheck.cpp
25 ↗(On Diff #122197)

I agree, removing a prefix is not a good idea. Warning is fine.

We could probably also change snake_case variables to CamelCase automatically. Not sure if it's worth doing in this review, but @Wizard can file a bug to follow up and add a TODO comment here mentioning the bug.

Wizard updated this revision to Diff 122319.Nov 9 2017, 1:13 PM
Wizard marked 6 inline comments as done.

address comments

clang-tidy/objc/PropertyDeclarationCheck.cpp
25 ↗(On Diff #122197)

Will do

hokein added inline comments.Nov 10 2017, 12:43 AM
clang-tidy/objc/PropertyDeclarationCheck.cpp
22 ↗(On Diff #122319)

I think we need to update the comment according to the new change (it is a bit ambiguous to users). We only fix "camelCase" currently.

25 ↗(On Diff #122319)

nit: llvm uses FIXME.

27 ↗(On Diff #122319)

nit: I would add an assert to make sure the name is not empty.

docs/ReleaseNotes.rst
63 ↗(On Diff #122319)

nit: this can be removed.

docs/clang-tidy/checks/objc-property-declaration.rst
22 ↗(On Diff #122319)

The same.

7 ↗(On Diff #122197)

I see, thanks for the clarification, I think the "Apple" would be clearer.

test/clang-tidy/objc-property-declaration.m
8 ↗(On Diff #122319)

Why does the check catch this case? Isn't camelCase a correct name?

Wizard added inline comments.Nov 10 2017, 2:19 AM
test/clang-tidy/objc-property-declaration.m
8 ↗(On Diff #122319)

This is CHECK-MESSAGES-NOT

benhamilton added inline comments.Nov 10 2017, 8:25 AM
clang-tidy/objc/PropertyDeclarationCheck.cpp
41 ↗(On Diff #122319)

There are some exceptions we should special case. Acronyms like URL and HTTP and HTML are allowed at the beginning of method names (and property names—although that is not explicitly mentioned, property names follow the same guidelines as method names):

https://developer.apple.com/library/content/documentation/Cocoa/Conceptual/CodingGuidelines/Articles/NamingBasics.html#//apple_ref/doc/uid/20001281-1002931-BBCFHEAB

For method names, start with a lowercase letter and capitalize the first letter of embedded words. Don’t use prefixes.
fileExistsAtPath:isDirectory:

An exception to this guideline is method names that start with a well-known acronym, for example, TIFFRepresentation (NSImage).

There is a list of well-known acronyms listed here:

https://developer.apple.com/library/content/documentation/Cocoa/Conceptual/CodingGuidelines/Articles/APIAbbreviations.html#//apple_ref/doc/uid/20001285-BCIHCGAE

ASCII
PDF
XML
HTML
URL
RTF
HTTP
TIFF
JPG
PNG
GIF
LZW
ROM
RGB
CMYK
MIDI
FTP
docs/clang-tidy/checks/google-objc-global-variable-declaration.rst
1 ↗(On Diff #122319)

Let's remove "google-" everywhere and mention only Apple's style guide.

docs/clang-tidy/checks/objc-property-declaration.rst
7 ↗(On Diff #122197)

Yeah, we don't want to mention Google here. Apple is good.

test/clang-tidy/objc-property-declaration.m
8 ↗(On Diff #122319)

I think in that case you don't need an explicit CHECK (there is implicitly a "CHECK-MESSAGES-NOT" on every line for all warnings which fails the test if a warning is raised).

Wizard updated this revision to Diff 122552.Nov 10 2017, 4:33 PM
Wizard marked 9 inline comments as done.

add custom options

Wizard marked 2 inline comments as done.Nov 10 2017, 4:33 PM
Wizard added inline comments.
clang-tidy/objc/PropertyDeclarationCheck.cpp
27 ↗(On Diff #122319)

Added it to the line before generating the warning message.

docs/clang-tidy/checks/google-objc-global-variable-declaration.rst
1 ↗(On Diff #122319)

Discussed offline. The change here is for another doc. Irrelevant to the new check.

hokein accepted this revision.Nov 13 2017, 12:31 AM

Looks good to me. I (or @benhamilton) will commit the patch for you if @benhamilton is fine with it.

clang-tidy/objc/PropertyDeclarationCheck.cpp
22 ↗(On Diff #122552)
93 ↗(On Diff #122552)

nit: clang-tidy message is not a complete sentence. just convertion; it should.

docs/clang-tidy/checks/objc-property-declaration.rst
40 ↗(On Diff #122552)

s/acronys/acronyms

test/clang-tidy/objc-property-declaration.m
8 ↗(On Diff #122319)

Sorry, I misread it as CHECK-MESSAGES. As Ben pointed out, we don't need explicit CHECK here (the default behavior is what we expected).

This revision is now accepted and ready to land.Nov 13 2017, 12:31 AM
Wizard updated this revision to Diff 122714.Nov 13 2017, 1:17 PM
Wizard marked 2 inline comments as done.

address nits

Wizard marked an inline comment as done.Nov 13 2017, 1:17 PM
Wizard edited the summary of this revision. (Show Details)Nov 13 2017, 1:38 PM
benhamilton added inline comments.Nov 13 2017, 1:39 PM
clang-tidy/objc/PropertyDeclarationCheck.cpp
89 ↗(On Diff #122714)

For the acronyms, this means we will allow properties with names like:

URLfoo_bar_blech

when we do not want that to be legal.

Instead, let's generate the regular expression we pass to matchesName() from the options. I will provide a suggestion shortly.

benhamilton edited edge metadata.
  • Use regex to match acronym prefixes, update tests

OK, I updated this diff with the suggested changes. @Wizard, want to take a look before I land?

This revision was automatically updated to reflect the committed changes.
hokein added inline comments.Nov 14 2017, 12:56 AM
clang-tidy/objc/PropertyDeclarationCheck.cpp
89 ↗(On Diff #122714)

Good catch.