This is an archive of the discontinued LLVM Phabricator instance.

Support special acronyms inside property names and allow plural forms
ClosedPublic

Authored by Wizard on Feb 5 2018, 10:37 PM.

Diff Detail

Event Timeline

Wizard created this revision.Feb 5 2018, 10:37 PM
Wizard updated this revision to Diff 132938.Feb 5 2018, 10:40 PM

resolve conflict in doc

hokein accepted this revision.Feb 6 2018, 1:33 AM

LGTM

This revision is now accepted and ready to land.Feb 6 2018, 1:33 AM
benhamilton requested changes to this revision.Feb 6 2018, 9:48 AM

Can you add test cases for non-plural acronyms in the middle of the string and plural acronyms at the start/end of the string, please?

clang-tidy/objc/PropertyDeclarationCheck.cpp
138–140

Why do we not allow plural acronyms at the start of the property name? For example:

@property(nonatomic) NSArray<NSString *> *URLsToFetch;

should be allowed.

140

Why do we not allow singular acronyms in the middle of the property name?

I think we should allow singular and plural acronyms anywhere.

This revision now requires changes to proceed.Feb 6 2018, 9:48 AM
Wizard updated this revision to Diff 133066.Feb 6 2018, 1:17 PM

resolve comments

clang-tidy/objc/PropertyDeclarationCheck.cpp
138–140

Hmm I was thinking that prefix should not have plural form. Will enable plural forms everywhere.

140

Actually we do. AcronymsGroupRegex(EscapedAcronyms, true) will support both while AcronymsGroupRegex(EscapedAcronyms, false) only supports singular. Will update test cases.

Wizard marked 2 inline comments as done.Feb 6 2018, 1:18 PM
benhamilton accepted this revision.Feb 6 2018, 1:31 PM
benhamilton added inline comments.
clang-tidy/objc/PropertyDeclarationCheck.cpp
138–140

Since AcronymsGroupRegex() is called twice with the same parameter, please store the result in a local variable instead of doing the work twice.

140

Ah, this is why I find boolean flags to be confusing :) Anyway, it's gone now.

test/clang-tidy/objc-property-declaration.m
12

Add a check for a plural at the end, please.

This revision is now accepted and ready to land.Feb 6 2018, 1:31 PM
Wizard updated this revision to Diff 133069.Feb 6 2018, 1:36 PM

minor fix according to comments

This revision was automatically updated to reflect the committed changes.