[PATCH] D39829: add new check for property declaration

[Haojian Wu via Phabricator via cfe-commits]

hokein added inline comments.



Comment at: clang-tidy/objc/PropertyDeclarationCheck.cpp:89
+  assert(MatchedDecl->getName().size() > 0);
+  // Skip the check of lowerCamelCase if the name has prefix of special 
acronyms
+  if (startsWithSpecialAcronyms(MatchedDecl->getName(), SpecialAcronyms)) {

benhamilton wrote:
> 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.
> 
Good catch.


Repository:
  rL LLVM

https://reviews.llvm.org/D39829



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D39829: add new check for property declaration

[Ben Hamilton via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL318117: add new check for property declaration (authored by 
benhamilton).

Repository:
  rL LLVM

https://reviews.llvm.org/D39829

Files:
  clang-tools-extra/trunk/clang-tidy/objc/CMakeLists.txt
  clang-tools-extra/trunk/clang-tidy/objc/ObjCTidyModule.cpp
  clang-tools-extra/trunk/clang-tidy/objc/PropertyDeclarationCheck.cpp
  clang-tools-extra/trunk/clang-tidy/objc/PropertyDeclarationCheck.h
  clang-tools-extra/trunk/docs/ReleaseNotes.rst
  
clang-tools-extra/trunk/docs/clang-tidy/checks/google-objc-global-variable-declaration.rst
  clang-tools-extra/trunk/docs/clang-tidy/checks/list.rst
  clang-tools-extra/trunk/docs/clang-tidy/checks/objc-property-declaration.rst
  clang-tools-extra/trunk/test/clang-tidy/objc-property-declaration-custom.m
  clang-tools-extra/trunk/test/clang-tidy/objc-property-declaration.m

Index: clang-tools-extra/trunk/clang-tidy/objc/PropertyDeclarationCheck.h
===
--- clang-tools-extra/trunk/clang-tidy/objc/PropertyDeclarationCheck.h
+++ clang-tools-extra/trunk/clang-tidy/objc/PropertyDeclarationCheck.h
@@ -0,0 +1,44 @@
+//===--- PropertyDeclarationCheck.h - clang-tidy-*- C++ -*-===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===--===//
+
+#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_OBJC_PROPERTY_DECLARATION_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_OBJC_PROPERTY_DECLARATION_H
+
+#include "../ClangTidy.h"
+#include 
+#include 
+
+namespace clang {
+namespace tidy {
+namespace objc {
+
+/// Finds Objective-C property declarations which
+/// are not in Lower Camel Case.
+///
+/// The format of property should look like:
+/// @property(nonatomic) NSString *lowerCamelCase;
+///
+/// For the user-facing documentation see:
+/// http://clang.llvm.org/extra/clang-tidy/checks/objc-property-declaration.html
+class PropertyDeclarationCheck : public ClangTidyCheck {
+public:
+  PropertyDeclarationCheck(StringRef Name, ClangTidyContext *Context);
+  void registerMatchers(ast_matchers::MatchFinder *Finder) override;
+  void check(const ast_matchers::MatchFinder::MatchResult ) override;
+  void storeOptions(ClangTidyOptions::OptionMap ) override;
+
+private:
+const std::vector SpecialAcronyms;
+};
+
+} // namespace objc
+} // namespace tidy
+} // namespace clang
+
+#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_OBJC_PROPERTY_DECLARATION_H
Index: clang-tools-extra/trunk/clang-tidy/objc/ObjCTidyModule.cpp
===
--- clang-tools-extra/trunk/clang-tidy/objc/ObjCTidyModule.cpp
+++ clang-tools-extra/trunk/clang-tidy/objc/ObjCTidyModule.cpp
@@ -11,6 +11,7 @@
 #include "../ClangTidyModule.h"
 #include "../ClangTidyModuleRegistry.h"
 #include "ForbiddenSubclassingCheck.h"
+#include "PropertyDeclarationCheck.h"
 
 using namespace clang::ast_matchers;
 
@@ -23,6 +24,8 @@
   void addCheckFactories(ClangTidyCheckFactories ) override {
 CheckFactories.registerCheck(
 "objc-forbidden-subclassing");
+CheckFactories.registerCheck(
+"objc-property-declaration");
   }
 };
 
Index: clang-tools-extra/trunk/clang-tidy/objc/CMakeLists.txt
===
--- clang-tools-extra/trunk/clang-tidy/objc/CMakeLists.txt
+++ clang-tools-extra/trunk/clang-tidy/objc/CMakeLists.txt
@@ -3,6 +3,7 @@
 add_clang_library(clangTidyObjCModule
   ForbiddenSubclassingCheck.cpp
   ObjCTidyModule.cpp
+  PropertyDeclarationCheck.cpp
 
   LINK_LIBS
   clangAST
Index: clang-tools-extra/trunk/clang-tidy/objc/PropertyDeclarationCheck.cpp
===
--- clang-tools-extra/trunk/clang-tidy/objc/PropertyDeclarationCheck.cpp
+++ clang-tools-extra/trunk/clang-tidy/objc/PropertyDeclarationCheck.cpp
@@ -0,0 +1,115 @@
+//===--- PropertyDeclarationCheck.cpp - clang-tidy-===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===--===//
+
+#include "PropertyDeclarationCheck.h"
+#include "../utils/OptionsUtils.h"
+#include "clang/AST/ASTContext.h"
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+#include "llvm/ADT/StringExtras.h"
+#include "llvm/Support/Regex.h"
+#include 
+
+using namespace clang::ast_matchers;
+
+namespace clang {
+namespace tidy {
+namespace objc {
+
+namespace {
+/// The acronyms are from
+/// 

[PATCH] D39829: add new check for property declaration

[Yan Zhang via Phabricator via cfe-commits]

Wizard added a comment.

LGTM


https://reviews.llvm.org/D39829



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D39829: add new check for property declaration

[Ben Hamilton via Phabricator via cfe-commits]

benhamilton added a comment.

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


https://reviews.llvm.org/D39829



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D39829: add new check for property declaration

[Ben Hamilton via Phabricator via cfe-commits]

benhamilton added inline comments.



Comment at: clang-tidy/objc/PropertyDeclarationCheck.cpp:89
+  assert(MatchedDecl->getName().size() > 0);
+  // Skip the check of lowerCamelCase if the name has prefix of special 
acronyms
+  if (startsWithSpecialAcronyms(MatchedDecl->getName(), SpecialAcronyms)) {

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.



https://reviews.llvm.org/D39829



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D39829: add new check for property declaration

[Yan Zhang via Phabricator via cfe-commits]

Wizard updated this revision to Diff 122714.
Wizard marked 2 inline comments as done.
Wizard added a comment.

address nits


https://reviews.llvm.org/D39829

Files:
  clang-tidy/objc/CMakeLists.txt
  clang-tidy/objc/ObjCTidyModule.cpp
  clang-tidy/objc/PropertyDeclarationCheck.cpp
  clang-tidy/objc/PropertyDeclarationCheck.h
  docs/ReleaseNotes.rst
  docs/clang-tidy/checks/google-objc-global-variable-declaration.rst
  docs/clang-tidy/checks/list.rst
  docs/clang-tidy/checks/objc-property-declaration.rst
  test/clang-tidy/objc-property-declaration-custom.m
  test/clang-tidy/objc-property-declaration.m

Index: test/clang-tidy/objc-property-declaration.m
===
--- /dev/null
+++ test/clang-tidy/objc-property-declaration.m
@@ -0,0 +1,10 @@
+// RUN: %check_clang_tidy %s objc-property-declaration %t
+@class NSString;
+
+@interface Foo
+@property(assign, nonatomic) int NotCamelCase;
+// CHECK-MESSAGES: :[[@LINE-1]]:34: warning: property 'NotCamelCase' is not in proper format according to property naming convention [objc-property-declaration]
+// CHECK-FIXES: @property(assign, nonatomic) int notCamelCase;
+@property(assign, nonatomic) int camelCase;
+@property(strong, nonatomic) NSString *URLString;
+@end
Index: test/clang-tidy/objc-property-declaration-custom.m
===
--- /dev/null
+++ test/clang-tidy/objc-property-declaration-custom.m
@@ -0,0 +1,11 @@
+// RUN: %check_clang_tidy %s objc-property-declaration %t \
+// RUN: -config='{CheckOptions: \
+// RUN:  [{key: objc-property-declaration.Acronyms, value: "ABC;TGIF"}]}' \
+// RUN: --
+
+@interface Foo
+@property(assign, nonatomic) int AbcNotRealPrefix;
+// CHECK-MESSAGES: :[[@LINE-1]]:34: warning: property 'AbcNotRealPrefix' is not in proper format according to property naming convention [objc-property-declaration]
+// CHECK-FIXES: @property(assign, nonatomic) int abcNotRealPrefix;
+@property(assign, nonatomic) int ABCCustomPrefix;
+@end
Index: docs/clang-tidy/checks/objc-property-declaration.rst
===
--- /dev/null
+++ docs/clang-tidy/checks/objc-property-declaration.rst
@@ -0,0 +1,43 @@
+.. title:: clang-tidy - objc-property-declaration
+
+objc-property-declaration
+=
+
+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.
+
+For code:
+
+.. code-block:: objc
+
+@property(nonatomic, assign) int LowerCamelCase;
+
+The fix will be:
+
+.. code-block:: objc
+
+@property(nonatomic, assign) int lowerCamelCase;
+
+The check will only fix 'CamelCase' to 'camelCase'. In some other cases we will
+only provide warning messages since the property name could be complicated.
+Users will need to come up with a proper name by their own.
+
+This check also accepts special acronyms as prefix. Such prefix will suppress
+the check of Lower Camel Case according to the guide:
+https://developer.apple.com/library/content/documentation/Cocoa/Conceptual/CodingGuidelines/Articles/NamingBasics.html#//apple_ref/doc/uid/20001281-1002931-BBCFHEAB
+
+For a full list of well-known acronyms:
+https://developer.apple.com/library/content/documentation/Cocoa/Conceptual/CodingGuidelines/Articles/APIAbbreviations.html#//apple_ref/doc/uid/20001285-BCIHCGAE
+
+The corresponding style rule: https://developer.apple.com/library/content/documentation/Cocoa/Conceptual/CodingGuidelines/Articles/NamingIvarsAndTypes.html#//apple_ref/doc/uid/20001284-1001757
+
+Options
+---
+
+.. option:: Acronyms
+
+   Semicolon-separated list of acronyms that can be used as prefix
+   of property names.
+
+   Defaults to ``ASCII;PDF;XML;HTML;URL;RTF;HTTP;TIFF;JPG;PNG;GIF;LZW;ROM;RGB;CMYK;MIDI;FTP``.
Index: docs/clang-tidy/checks/list.rst
===
--- docs/clang-tidy/checks/list.rst
+++ docs/clang-tidy/checks/list.rst
@@ -173,15 +173,16 @@
modernize-use-using
mpi-buffer-deref
mpi-type-mismatch
+   objc-forbidden-subclassing
+   objc-property-declaration
performance-faster-string-find
performance-for-range-copy
performance-implicit-conversion-in-loop
performance-inefficient-string-concatenation
performance-inefficient-vector-operation
performance-type-promotion-in-math-fn
performance-unnecessary-copy-initialization
performance-unnecessary-value-param
-   objc-forbidden-subclassing
readability-avoid-const-params-in-decls
readability-braces-around-statements
readability-container-size-empty
Index: docs/clang-tidy/checks/google-objc-global-variable-declaration.rst
===
--- docs/clang-tidy/checks/google-objc-global-variable-declaration.rst
+++ 

[PATCH] D39829: add new check for property declaration

[Haojian Wu via Phabricator via cfe-commits]

hokein accepted this revision.
hokein added a comment.
This revision is now accepted and ready to land.

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




Comment at: clang-tidy/objc/PropertyDeclarationCheck.cpp:22
+namespace {
+constexpr char DefaultSpecialAcronyms[] =
+"ASCII;"

nit: add a comment documenting these are from 
https://developer.apple.com/library/content/documentation/Cocoa/Conceptual/CodingGuidelines/Articles/APIAbbreviations.html#//apple_ref/doc/uid/20001285-BCIHCGAE.



Comment at: clang-tidy/objc/PropertyDeclarationCheck.cpp:93
+   "property '%0' is not in proper format according to property naming "
+   "convention. It should be in the format of lowerCamelCase or has "
+   "special acronyms")

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



Comment at: docs/clang-tidy/checks/objc-property-declaration.rst:40
+
+   Semicolon-separated list of acronys that can be used as prefix
+   of property names.

s/acronys/acronyms



Comment at: test/clang-tidy/objc-property-declaration.m:8
+@property(assign, nonatomic) int camelCase;
+// CHECK-MESSAGES-NOT: :[[@LINE-1]]:34: warning: property 'camelCase' is not 
in proper format according to property naming convention 
[objc-property-declaration]
+@end

benhamilton wrote:
> Wizard wrote:
> > hokein wrote:
> > > Why does the check catch this case? Isn't `camelCase` a correct name?
> > This is CHECK-MESSAGES-NOT
> 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).
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).


https://reviews.llvm.org/D39829



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D39829: add new check for property declaration

[Yan Zhang via Phabricator via cfe-commits]

Wizard marked 2 inline comments as done.
Wizard added inline comments.



Comment at: clang-tidy/objc/PropertyDeclarationCheck.cpp:27
+FixItHint generateFixItHint(const ObjCPropertyDecl *Decl) {
+  if (isupper(Decl->getName()[0])) {
+auto NewName = Decl->getName().str();

hokein wrote:
> nit: I would add an assert to make sure the name is not empty.
Added it to the line before generating the warning message.



Comment at: docs/clang-tidy/checks/google-objc-global-variable-declaration.rst:1
 .. title:: clang-tidy - google-objc-global-variable-declaration
 

benhamilton wrote:
> Let's remove "google-" everywhere and mention only Apple's style guide.
Discussed offline. The change here is for another doc. Irrelevant to the new 
check.


https://reviews.llvm.org/D39829



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D39829: add new check for property declaration

[Yan Zhang via Phabricator via cfe-commits]

Wizard updated this revision to Diff 122552.
Wizard marked 9 inline comments as done.
Wizard added a comment.

add custom options


https://reviews.llvm.org/D39829

Files:
  clang-tidy/objc/CMakeLists.txt
  clang-tidy/objc/ObjCTidyModule.cpp
  clang-tidy/objc/PropertyDeclarationCheck.cpp
  clang-tidy/objc/PropertyDeclarationCheck.h
  docs/ReleaseNotes.rst
  docs/clang-tidy/checks/google-objc-global-variable-declaration.rst
  docs/clang-tidy/checks/list.rst
  docs/clang-tidy/checks/objc-property-declaration.rst
  test/clang-tidy/objc-property-declaration-custom.m
  test/clang-tidy/objc-property-declaration.m

Index: test/clang-tidy/objc-property-declaration.m
===
--- /dev/null
+++ test/clang-tidy/objc-property-declaration.m
@@ -0,0 +1,10 @@
+// RUN: %check_clang_tidy %s objc-property-declaration %t
+@class NSString;
+
+@interface Foo
+@property(assign, nonatomic) int NotCamelCase;
+// CHECK-MESSAGES: :[[@LINE-1]]:34: warning: property 'NotCamelCase' is not in proper format according to property naming convention. It should be in the format of lowerCamelCase or has special acronyms [objc-property-declaration]
+// CHECK-FIXES: @property(assign, nonatomic) int notCamelCase;
+@property(assign, nonatomic) int camelCase;
+@property(strong, nonatomic) NSString *URLString;
+@end
Index: test/clang-tidy/objc-property-declaration-custom.m
===
--- /dev/null
+++ test/clang-tidy/objc-property-declaration-custom.m
@@ -0,0 +1,11 @@
+// RUN: %check_clang_tidy %s objc-property-declaration %t \
+// RUN: -config='{CheckOptions: \
+// RUN:  [{key: objc-property-declaration.Acronyms, value: "ABC;TGIF"}]}' \
+// RUN: --
+
+@interface Foo
+@property(assign, nonatomic) int AbcNotRealPrefix;
+// CHECK-MESSAGES: :[[@LINE-1]]:34: warning: property 'AbcNotRealPrefix' is not in proper format according to property naming convention. It should be in the format of lowerCamelCase or has special acronyms [objc-property-declaration]
+// CHECK-FIXES: @property(assign, nonatomic) int abcNotRealPrefix;
+@property(assign, nonatomic) int ABCCustomPrefix;
+@end
Index: docs/clang-tidy/checks/objc-property-declaration.rst
===
--- /dev/null
+++ docs/clang-tidy/checks/objc-property-declaration.rst
@@ -0,0 +1,43 @@
+.. title:: clang-tidy - objc-property-declaration
+
+objc-property-declaration
+=
+
+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.
+
+For code:
+
+.. code-block:: objc
+
+@property(nonatomic, assign) int LowerCamelCase;
+
+The fix will be:
+
+.. code-block:: objc
+
+@property(nonatomic, assign) int lowerCamelCase;
+
+The check will only fix 'CamelCase' to 'camelCase'. In some other cases we will
+only provide warning messages since the property name could be complicated.
+Users will need to come up with a proper name by their own.
+
+This check also accepts special acronyms as prefix. Such prefix will suppress
+the check of Lower Camel Case according to the guide:
+https://developer.apple.com/library/content/documentation/Cocoa/Conceptual/CodingGuidelines/Articles/NamingBasics.html#//apple_ref/doc/uid/20001281-1002931-BBCFHEAB
+
+For a full list of well-known acronyms:
+https://developer.apple.com/library/content/documentation/Cocoa/Conceptual/CodingGuidelines/Articles/APIAbbreviations.html#//apple_ref/doc/uid/20001285-BCIHCGAE
+
+The corresponding style rule: https://developer.apple.com/library/content/documentation/Cocoa/Conceptual/CodingGuidelines/Articles/NamingIvarsAndTypes.html#//apple_ref/doc/uid/20001284-1001757
+
+Options
+---
+
+.. option:: Acronyms
+
+   Semicolon-separated list of acronys that can be used as prefix
+   of property names.
+
+   Defaults to ``ASCII;PDF;XML;HTML;URL;RTF;HTTP;TIFF;JPG;PNG;GIF;LZW;ROM;RGB;CMYK;MIDI;FTP``.
Index: docs/clang-tidy/checks/list.rst
===
--- docs/clang-tidy/checks/list.rst
+++ docs/clang-tidy/checks/list.rst
@@ -173,15 +173,16 @@
modernize-use-using
mpi-buffer-deref
mpi-type-mismatch
+   objc-forbidden-subclassing
+   objc-property-declaration
performance-faster-string-find
performance-for-range-copy
performance-implicit-conversion-in-loop
performance-inefficient-string-concatenation
performance-inefficient-vector-operation
performance-type-promotion-in-math-fn
performance-unnecessary-copy-initialization
performance-unnecessary-value-param
-   objc-forbidden-subclassing
readability-avoid-const-params-in-decls
readability-braces-around-statements
readability-container-size-empty
Index: docs/clang-tidy/checks/google-objc-global-variable-declaration.rst

[PATCH] D39829: add new check for property declaration

[Ben Hamilton via Phabricator via cfe-commits]

benhamilton added inline comments.



Comment at: clang-tidy/objc/PropertyDeclarationCheck.cpp:41
+  objcPropertyDecl(
+  // the property name should be in Lower Camel Case like
+  // 'lowerCamelCase'

benhamilton wrote:
> 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
> ```
> 
Probably the right thing to do is make this a configuration option, the same 
way I did with `objc-forbidden-subclassing`, and default to the above list.

https://github.com/llvm-mirror/clang-tools-extra/blob/master/clang-tidy/objc/ForbiddenSubclassingCheck.cpp#L25
https://github.com/llvm-mirror/clang-tools-extra/blob/master/clang-tidy/objc/ForbiddenSubclassingCheck.cpp#L74
https://github.com/llvm-mirror/clang-tools-extra/blob/master/clang-tidy/objc/ForbiddenSubclassingCheck.cpp#L110

 


https://reviews.llvm.org/D39829



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D39829: add new check for property declaration

[Ben Hamilton via Phabricator via cfe-commits]

benhamilton added inline comments.



Comment at: clang-tidy/objc/PropertyDeclarationCheck.cpp:41
+  objcPropertyDecl(
+  // the property name should be in Lower Camel Case like
+  // 'lowerCamelCase'

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
```




Comment at: docs/clang-tidy/checks/google-objc-global-variable-declaration.rst:1
 .. title:: clang-tidy - google-objc-global-variable-declaration
 

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



Comment at: docs/clang-tidy/checks/objc-property-declaration.rst:7
+Finds property declarations in Objective-C files that do not follow the pattern
+of property names in Google's Objective-C Style Guide. The property name should
+be in the format of Lower Camel Case.

hokein wrote:
> benhamilton wrote:
> > hokein wrote:
> > > Google style? but the link you provided is Apple.
> > 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.
> I see, thanks for the clarification, I think the "Apple" would be clearer.
Yeah, we don't want to mention Google here. Apple is good.



Comment at: test/clang-tidy/objc-property-declaration.m:8
+@property(assign, nonatomic) int camelCase;
+// CHECK-MESSAGES-NOT: :[[@LINE-1]]:34: warning: property 'camelCase' is not 
in proper format according to property naming convention 
[objc-property-declaration]
+@end

Wizard wrote:
> hokein wrote:
> > Why does the check catch this case? Isn't `camelCase` a correct name?
> This is CHECK-MESSAGES-NOT
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).


https://reviews.llvm.org/D39829



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D39829: add new check for property declaration

[Yan Zhang via Phabricator via cfe-commits]

Wizard added inline comments.



Comment at: test/clang-tidy/objc-property-declaration.m:8
+@property(assign, nonatomic) int camelCase;
+// CHECK-MESSAGES-NOT: :[[@LINE-1]]:34: warning: property 'camelCase' is not 
in proper format according to property naming convention 
[objc-property-declaration]
+@end

hokein wrote:
> Why does the check catch this case? Isn't `camelCase` a correct name?
This is CHECK-MESSAGES-NOT


https://reviews.llvm.org/D39829



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D39829: add new check for property declaration

[Haojian Wu via Phabricator via cfe-commits]

hokein added inline comments.



Comment at: clang-tidy/objc/PropertyDeclarationCheck.cpp:22
+
+/// we will do best effort to generate a fix, however, if the
+/// case can not be solved with a simple fix (e.g. CamelCase

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.



Comment at: clang-tidy/objc/PropertyDeclarationCheck.cpp:25
+/// is a simple fix), we will leave the fix to the user.
+/// TODO: provide fix for snake_case to snakeCase
+FixItHint generateFixItHint(const ObjCPropertyDecl *Decl) {

nit: llvm uses `FIXME`.



Comment at: clang-tidy/objc/PropertyDeclarationCheck.cpp:27
+FixItHint generateFixItHint(const ObjCPropertyDecl *Decl) {
+  if (isupper(Decl->getName()[0])) {
+auto NewName = Decl->getName().str();

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



Comment at: docs/ReleaseNotes.rst:63
+
+  FIXME: add release notes.
+

nit:  this can be removed.



Comment at: docs/clang-tidy/checks/objc-property-declaration.rst:7
+Finds property declarations in Objective-C files that do not follow the pattern
+of property names in Google's Objective-C Style Guide. The property name should
+be in the format of Lower Camel Case.

benhamilton wrote:
> hokein wrote:
> > Google style? but the link you provided is Apple.
> 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.
I see, thanks for the clarification, I think the "Apple" would be clearer.



Comment at: docs/clang-tidy/checks/objc-property-declaration.rst:22
+
+The check will do best effort to give a fix, however, in some cases it is
+difficult to give a proper fix since the property name could be complicated.

The same.



Comment at: test/clang-tidy/objc-property-declaration.m:8
+@property(assign, nonatomic) int camelCase;
+// CHECK-MESSAGES-NOT: :[[@LINE-1]]:34: warning: property 'camelCase' is not 
in proper format according to property naming convention 
[objc-property-declaration]
+@end

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


https://reviews.llvm.org/D39829



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D39829: add new check for property declaration

[Yan Zhang via Phabricator via cfe-commits]

Wizard added inline comments.



Comment at: clang-tidy/objc/PropertyDeclarationCheck.cpp:25
+/// we will do best effort to generate a fix, however, if the
+/// case can not be solved with a simple fix (e.g. remove prefix or change 
first
+/// character), we will leave the fix to the user.

benhamilton wrote:
> hokein wrote:
> > 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).
> 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.
Will do


https://reviews.llvm.org/D39829



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D39829: add new check for property declaration

[Yan Zhang via Phabricator via cfe-commits]

Wizard updated this revision to Diff 122319.
Wizard marked 6 inline comments as done.
Wizard added a comment.

address comments


https://reviews.llvm.org/D39829

Files:
  clang-tidy/objc/CMakeLists.txt
  clang-tidy/objc/ObjCTidyModule.cpp
  clang-tidy/objc/PropertyDeclarationCheck.cpp
  clang-tidy/objc/PropertyDeclarationCheck.h
  docs/ReleaseNotes.rst
  docs/clang-tidy/checks/google-objc-global-variable-declaration.rst
  docs/clang-tidy/checks/list.rst
  docs/clang-tidy/checks/objc-property-declaration.rst
  test/clang-tidy/objc-property-declaration.m

Index: test/clang-tidy/objc-property-declaration.m
===
--- /dev/null
+++ test/clang-tidy/objc-property-declaration.m
@@ -0,0 +1,9 @@
+// RUN: %check_clang_tidy %s objc-property-declaration %t
+
+@interface Foo
+@property(assign, nonatomic) int NotCamelCase;
+// CHECK-MESSAGES: :[[@LINE-1]]:34: warning: property 'NotCamelCase' is not in proper format according to property naming convention [objc-property-declaration]
+// CHECK-FIXES: @property(assign, nonatomic) int notCamelCase;
+@property(assign, nonatomic) int camelCase;
+// CHECK-MESSAGES-NOT: :[[@LINE-1]]:34: warning: property 'camelCase' is not in proper format according to property naming convention [objc-property-declaration]
+@end
Index: docs/clang-tidy/checks/objc-property-declaration.rst
===
--- /dev/null
+++ docs/clang-tidy/checks/objc-property-declaration.rst
@@ -0,0 +1,26 @@
+.. title:: clang-tidy - objc-property-declaration
+
+objc-property-declaration
+=
+
+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.
+
+For code:
+
+.. code-block:: objc
+
+@property(nonatomic, assign) int LowerCamelCase;
+
+The fix will be:
+
+.. code-block:: objc
+
+@property(nonatomic, assign) int lowerCamelCase;
+
+The check will do best effort to give a fix, however, in some cases it is
+difficult to give a proper fix since the property name could be complicated.
+Users will need to come up with a proper name by their own.
+
+The corresponding style rule: https://developer.apple.com/library/content/documentation/Cocoa/Conceptual/CodingGuidelines/Articles/NamingIvarsAndTypes.html#//apple_ref/doc/uid/20001284-1001757
Index: docs/clang-tidy/checks/list.rst
===
--- docs/clang-tidy/checks/list.rst
+++ docs/clang-tidy/checks/list.rst
@@ -173,15 +173,16 @@
modernize-use-using
mpi-buffer-deref
mpi-type-mismatch
+   objc-forbidden-subclassing
+   objc-property-declaration
performance-faster-string-find
performance-for-range-copy
performance-implicit-conversion-in-loop
performance-inefficient-string-concatenation
performance-inefficient-vector-operation
performance-type-promotion-in-math-fn
performance-unnecessary-copy-initialization
performance-unnecessary-value-param
-   objc-forbidden-subclassing
readability-avoid-const-params-in-decls
readability-braces-around-statements
readability-container-size-empty
Index: docs/clang-tidy/checks/google-objc-global-variable-declaration.rst
===
--- docs/clang-tidy/checks/google-objc-global-variable-declaration.rst
+++ docs/clang-tidy/checks/google-objc-global-variable-declaration.rst
@@ -3,7 +3,7 @@
 google-objc-global-variable-declaration
 ===
 
-Finds global variable declarations in Objective-C files that are not follow the pattern
+Finds global variable declarations in Objective-C files that do not follow the pattern
 of variable names in Google's Objective-C Style Guide.
 
 The corresponding style guide rule:
@@ -16,26 +16,31 @@
 For code:
 
 .. code-block:: objc
+
   static NSString* myString = @"hello";
 
 The fix will be:
 
 .. code-block:: objc
+
   static NSString* gMyString = @"hello";
 
 Another example of constant:
 
 .. code-block:: objc
+
   static NSString* const myConstString = @"hello";
 
 The fix will be:
 
 .. code-block:: objc
+
   static NSString* const kMyConstString = @"hello";
 
 However for code that prefixed with non-alphabetical characters like:
 
 .. code-block:: objc
+
   static NSString* __anotherString = @"world";
 
 The check will give a warning message but will not be able to suggest a fix. The user
Index: docs/ReleaseNotes.rst
===
--- docs/ReleaseNotes.rst
+++ docs/ReleaseNotes.rst
@@ -57,6 +57,11 @@
 Improvements to clang-tidy
 --
 
+- New `objc-property-declaration
+  `_ check
+
+  FIXME: add release notes.
+
 - New `google-objc-global-variable-declaration
   

[PATCH] D39829: add new check for property declaration

[Ben Hamilton via Phabricator via cfe-commits]

benhamilton added inline comments.



Comment at: clang-tidy/objc/PropertyDeclarationCheck.cpp:25
+/// we will do best effort to generate a fix, however, if the
+/// case can not be solved with a simple fix (e.g. remove prefix or change 
first
+/// character), we will leave the fix to the user.

hokein wrote:
> 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).
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.


https://reviews.llvm.org/D39829



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D39829: add new check for property declaration

[Ben Hamilton via Phabricator via cfe-commits]

benhamilton added inline comments.



Comment at: docs/clang-tidy/checks/objc-property-declaration.rst:7
+Finds property declarations in Objective-C files that do not follow the pattern
+of property names in Google's Objective-C Style Guide. The property name should
+be in the format of Lower Camel Case.

hokein wrote:
> Google style? but the link you provided is Apple.
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.


https://reviews.llvm.org/D39829



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D39829: add new check for property declaration

[Haojian Wu via Phabricator via cfe-commits]

hokein added a comment.

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




Comment at: clang-tidy/objc/PropertyDeclarationCheck.cpp:14
+
+#include 
+

Do we need this header?



Comment at: clang-tidy/objc/PropertyDeclarationCheck.cpp:25
+/// we will do best effort to generate a fix, however, if the
+/// case can not be solved with a simple fix (e.g. remove prefix or change 
first
+/// character), we will leave the fix to the user.

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).



Comment at: docs/clang-tidy/checks/objc-property-declaration.rst:7
+Finds property declarations in Objective-C files that do not follow the pattern
+of property names in Google's Objective-C Style Guide. The property name should
+be in the format of Lower Camel Case.

Google style? but the link you provided is Apple.



Comment at: docs/clang-tidy/checks/objc-property-declaration.rst:27
+The corresponding style rule: 
https://developer.apple.com/library/content/documentation/Cocoa/Conceptual/CodingGuidelines/Articles/NamingIvarsAndTypes.html#//apple_ref/doc/uid/20001284-1001757
\ No newline at end of file


nit: add a newline.


https://reviews.llvm.org/D39829



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits