[PATCH] D42464: add prefix with '_' support for property name. Corresponding apple dev doc: https://developer.apple.com/library/content/qa/qa1908/_index.html

[Ben Hamilton via Phabricator via cfe-commits]

benhamilton added inline comments.


================
Comment at: clang-tidy/objc/PropertyDeclarationCheck.cpp:52
 /// FIXME: provide fix for snake_case to snakeCase
-FixItHint generateFixItHint(const ObjCPropertyDecl *Decl) {
-  if (isupper(Decl->getName()[0])) {
-    auto NewName = Decl->getName().str();
-    NewName[0] = tolower(NewName[0]);
-    return FixItHint::CreateReplacement(
-        CharSourceRange::getTokenRange(SourceRange(Decl->getLocation())),
-        llvm::StringRef(NewName));
+FixItHint generateFixItHint(const ObjCPropertyDecl *Decl, bool isCategory) {
+  auto Name = Decl->getName();
----------------
Instead of bool isCategory, how about an enum NamingStyle:

```
enum NamingStyle {
  StandardProperty = 1,
  CategoryProperty = 2,
}
```



================
Comment at: clang-tidy/objc/PropertyDeclarationCheck.cpp:102
+bool hasCategoryPropertyPrefix(const llvm::StringRef &PropertyName) {
+  for (size_t i = 0; i < PropertyName.size() - 1; ++i) {
+    if (PropertyName[i] == '_') {
----------------
I think this is an off-by-one error, right? Change:

  i < PropertyName.size() - 1

to

  i < PropertyName.size()



================
Comment at: clang-tidy/objc/PropertyDeclarationCheck.cpp:103-105
+    if (PropertyName[i] == '_') {
+      return true;
+    }
----------------
Do we really want a leading _ to count? I think this might need to be a regular expression instead, something like:

  ^[a-zA-Z][a-zA-Z0-9]+_[a-zA-Z0-9][a-zA-Z0-9_]*$



================
Comment at: clang-tidy/objc/PropertyDeclarationCheck.cpp:120
+  auto RegexExp = llvm::Regex(
+      llvm::StringRef(validPropertyNameRegex(Acronyms).replace(0, 2, "^")));
+  return RegexExp.match(llvm::StringRef(PropertyName.substr(Start + 1)));
----------------
I don't understand what this is doing. Why are we replacing things in a regex?

I don't think this is very maintainable. Can you rewrite it for legibility, please?


================
Comment at: clang-tidy/objc/PropertyDeclarationCheck.cpp:147
+  auto NodeKind = std::string(ParentContext->getDeclKindName());
+  if (NodeKind == "ObjCCategory" &&
+      hasCategoryPropertyPrefix(MatchedDecl->getName())) {
----------------
Is a class extension treated as a category? If so, we probably need to treat it specially.

e.g.:

Foo.h:

```
@interface Foo
@end
```

Foo.m:

```
@interface Foo ()
@property (nonatomic, readonly) int foo_bar;
@end
```

should not be allowed.

Can you handle this and add tests, please?


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D42464