This is an archive of the discontinued LLVM Phabricator instance.

Add support for ObjC property name to be a single acronym.
ClosedPublic

Authored by Wizard on May 2 2018, 6:44 PM.

Diff Detail

Repository
rL LLVM

Event Timeline

Wizard created this revision.May 2 2018, 6:44 PM
Wizard edited the summary of this revision. (Show Details)May 2 2018, 6:46 PM
Wizard added reviewers: benhamilton, hokein.
Wizard added a project: Restricted Project.
Wizard updated this revision to Diff 144970.May 2 2018, 6:48 PM
Wizard edited the summary of this revision. (Show Details)

fix format

benhamilton requested changes to this revision.May 3 2018, 8:14 AM
benhamilton added inline comments.
clang-tidy/objc/PropertyDeclarationCheck.cpp
222 ↗(On Diff #144970)

s is a regular expression here, so you need to match it using llvm::Regex, not ==.

Why not just update validPropertyNameRegex() to handle this case?

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

Please add a test for a built-in regex (4G) as well as a custom regex in the other test file.

This revision now requires changes to proceed.May 3 2018, 8:14 AM
Wizard added inline comments.May 3 2018, 1:48 PM
clang-tidy/objc/PropertyDeclarationCheck.cpp
222 ↗(On Diff #144970)

I would rather not to make the regex more complex as long as the change is simple and does bring extra cost. If update the regex it will be something like '(originalregex | acronyms)', which seems too much to me.

Wizard updated this revision to Diff 145145.May 3 2018, 8:58 PM

resolve comments

Wizard marked 3 inline comments as done.May 3 2018, 8:59 PM
Wizard added inline comments.
test/clang-tidy/objc-property-declaration.m
24 ↗(On Diff #144970)

Unable to add single property test of 4G because it is illegal to use digit as the first character of property name.

Wizard marked an inline comment as done.May 3 2018, 9:00 PM
benhamilton accepted this revision.May 4 2018, 8:17 AM
benhamilton added inline comments.
clang-tidy/objc/PropertyDeclarationCheck.cpp
222 ↗(On Diff #145145)

Please be aware this will change the match from running a single regular expression to running ~ 70 regular expressions on every single @property. I would expect this to perform pretty poorly.

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

Ah, of course.

This revision is now accepted and ready to land.May 4 2018, 8:17 AM
Wizard updated this revision to Diff 145226.May 4 2018, 11:14 AM

optimize matching

Wizard marked an inline comment as done.May 4 2018, 11:15 AM
Wizard added inline comments.
clang-tidy/objc/PropertyDeclarationCheck.cpp
222 ↗(On Diff #145145)

This is a good point. If using matching instead of equality check, I should not use any_of then. Updated it to use AcronymsGroupRegex().

This revision was automatically updated to reflect the committed changes.
Wizard marked an inline comment as done.