[PATCH] D45392: [clang-tidy] add new check to find out objc ivars which do not have prefix '_'

[Yan Zhang via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rCTE330492: [clang-tidy] add new check to find out objc ivars 
which do not have prefix _ (authored by Wizard, committed by ).

Changed prior to commit:
  https://reviews.llvm.org/D45392?vs=142057=143408#toc

Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D45392

Files:
  clang-tidy/readability/IdentifierNamingCheck.cpp
  test/clang-tidy/readability-identifier-naming-objc.m


Index: test/clang-tidy/readability-identifier-naming-objc.m
===
--- test/clang-tidy/readability-identifier-naming-objc.m
+++ test/clang-tidy/readability-identifier-naming-objc.m
@@ -0,0 +1,15 @@
+// RUN: %check_clang_tidy %s readability-identifier-naming %t \
+// RUN: -config='{CheckOptions: \
+// RUN:  [{key: readability-identifier-naming.ObjcIvarPrefix, value: '_'}]}' \
+// RUN: --
+
+@interface Foo
+@end 
+
+@interface Foo () {
+int _bar;
+int barWithoutPrefix;
+// CHECK-MESSAGES: :[[@LINE-1]]:9: warning: invalid case style for objc 
ivar 'barWithoutPrefix' [readability-identifier-naming]
+// CHECK-FIXES: int _barWithoutPrefix;
+}
+@end
Index: clang-tidy/readability/IdentifierNamingCheck.cpp
===
--- clang-tidy/readability/IdentifierNamingCheck.cpp
+++ clang-tidy/readability/IdentifierNamingCheck.cpp
@@ -109,6 +109,7 @@
 m(TemplateParameter) \
 m(TypeAlias) \
 m(MacroDefinition) \
+m(ObjcIvar) \
 
 enum StyleKind {
 #define ENUMERATE(v) SK_ ## v,
@@ -384,6 +385,9 @@
 const NamedDecl *D,
 const std::vector
 ) {
+  if (isa(D) && NamingStyles[SK_ObjcIvar])
+return SK_ObjcIvar;
+
   if (isa(D) && NamingStyles[SK_Typedef])
 return SK_Typedef;
 


Index: test/clang-tidy/readability-identifier-naming-objc.m
===
--- test/clang-tidy/readability-identifier-naming-objc.m
+++ test/clang-tidy/readability-identifier-naming-objc.m
@@ -0,0 +1,15 @@
+// RUN: %check_clang_tidy %s readability-identifier-naming %t \
+// RUN: -config='{CheckOptions: \
+// RUN:  [{key: readability-identifier-naming.ObjcIvarPrefix, value: '_'}]}' \
+// RUN: --
+
+@interface Foo
+@end 
+
+@interface Foo () {
+int _bar;
+int barWithoutPrefix;
+// CHECK-MESSAGES: :[[@LINE-1]]:9: warning: invalid case style for objc ivar 'barWithoutPrefix' [readability-identifier-naming]
+// CHECK-FIXES: int _barWithoutPrefix;
+}
+@end
Index: clang-tidy/readability/IdentifierNamingCheck.cpp
===
--- clang-tidy/readability/IdentifierNamingCheck.cpp
+++ clang-tidy/readability/IdentifierNamingCheck.cpp
@@ -109,6 +109,7 @@
 m(TemplateParameter) \
 m(TypeAlias) \
 m(MacroDefinition) \
+m(ObjcIvar) \
 
 enum StyleKind {
 #define ENUMERATE(v) SK_ ## v,
@@ -384,6 +385,9 @@
 const NamedDecl *D,
 const std::vector
 ) {
+  if (isa(D) && NamingStyles[SK_ObjcIvar])
+return SK_ObjcIvar;
+
   if (isa(D) && NamingStyles[SK_Typedef])
 return SK_Typedef;
 
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D45392: [clang-tidy] add new check to find out objc ivars which do not have prefix '_'

[Alexander Kornienko via Phabricator via cfe-commits]

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

LG. Thanks!


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D45392



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

[PATCH] D45392: [clang-tidy] add new check to find out objc ivars which do not have prefix '_'

[Yan Zhang via Phabricator via cfe-commits]

Wizard added inline comments.



Comment at: test/clang-tidy/readability-identifier-naming-objc.m:4
+// RUN:  [{key: readability-identifier-naming.ObjcIvarPrefix, value: '_'}]}' \
+// RUN: --
+

alexfh wrote:
> The `--` and the trailing backslash above can be removed as well.
I tried but when I run the test there some error output:
Running ['clang-tidy', 
'/Users/ynzhang/clang-llvm/build/tools/clang/tools/extra/test/clang-tidy/Output/readability-identifier-naming-objc.m.tmp.m',
 '-fix', '--checks=-*,readability-identifier-naming', '-config={CheckOptions:   
[{key: readability-identifier-naming.ObjcIvarPrefix, value: _}]}', 
'-nostdinc++']...
clang-tidy failed:
LLVM ERROR: CommonOptionsParser: failed to parse command-line arguments. 
[CommonOptionsParser]: clang-tidy: Unknown command line argument '-nostdinc++'. 
 Try: 'clang-tidy -help'
clang-tidy: Did you mean '-gpsize'?

No idea where '-nostdinc++' came from, but there is no such problem with these 
stuff added. I actually followed the command pattern in 
objc-forbidden-subclassing-custom.m


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D45392



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

[PATCH] D45392: [clang-tidy] add new check to find out objc ivars which do not have prefix '_'

[Alexander Kornienko via Phabricator via cfe-commits]

alexfh added a comment.

BTW, it looks like the 
http://clang.llvm.org/extra/clang-tidy/checks/objc-property-declaration.html 
check could also be replaced with readability-identifier-naming (not in this 
patch).


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D45392



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

[PATCH] D45392: [clang-tidy] add new check to find out objc ivars which do not have prefix '_'

[Alexander Kornienko via Phabricator via cfe-commits]

alexfh added inline comments.



Comment at: test/clang-tidy/readability-identifier-naming-objc.m:8-9
+// RUN:   ]}' -- -fno-delayed-template-parsing \
+// RUN:   -I%S/Inputs/readability-identifier-naming \
+// RUN:   -isystem %S/Inputs/readability-identifier-naming/system
+

Wizard wrote:
> alexfh wrote:
> > It looks like these flags are not needed, since there are no #include 
> > directives in this test.
> So we won't worry about those flags when using this check in google codebase 
> right?
These flags are used by the other test to specify directories used  to search 
for #included headers. Since the test is copied to a temporary directory by the 
check_clang_tidy.py script and the headers stay in the source tree (see the 
test/clang-tidy/Inputs/readability-identifier-naming/ directory), the test 
can't just use relative include paths, instead we specify the full path using 
the -I and -isystem compiler options. In real use cases all compilation 
arguments (including the required `-I` and `-isystem` options) are taken from a 
compilation database.  No need to specify them in clang-tidy configuration.



Comment at: test/clang-tidy/readability-identifier-naming-objc.m:4
+// RUN:  [{key: readability-identifier-naming.ObjcIvarPrefix, value: '_'}]}' \
+// RUN: --
+

The `--` and the trailing backslash above can be removed as well.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D45392



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

[PATCH] D45392: [clang-tidy] add new check to find out objc ivars which do not have prefix '_'

[Yan Zhang via Phabricator via cfe-commits]

Wizard added inline comments.



Comment at: test/clang-tidy/readability-identifier-naming-objc.m:8-9
+// RUN:   ]}' -- -fno-delayed-template-parsing \
+// RUN:   -I%S/Inputs/readability-identifier-naming \
+// RUN:   -isystem %S/Inputs/readability-identifier-naming/system
+

alexfh wrote:
> It looks like these flags are not needed, since there are no #include 
> directives in this test.
So we won't worry about those flags when using this check in google codebase 
right?


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D45392



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

[PATCH] D45392: [clang-tidy] add new check to find out objc ivars which do not have prefix '_'

[Yan Zhang via Phabricator via cfe-commits]

Wizard updated this revision to Diff 142057.
Wizard added a comment.

remove unnecessary flags


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D45392

Files:
  clang-tidy/readability/IdentifierNamingCheck.cpp
  test/clang-tidy/readability-identifier-naming-objc.m


Index: test/clang-tidy/readability-identifier-naming-objc.m
===
--- /dev/null
+++ test/clang-tidy/readability-identifier-naming-objc.m
@@ -0,0 +1,15 @@
+// RUN: %check_clang_tidy %s readability-identifier-naming %t \
+// RUN: -config='{CheckOptions: \
+// RUN:  [{key: readability-identifier-naming.ObjcIvarPrefix, value: '_'}]}' \
+// RUN: --
+
+@interface Foo
+@end 
+
+@interface Foo () {
+int _bar;
+int barWithoutPrefix;
+// CHECK-MESSAGES: :[[@LINE-1]]:9: warning: invalid case style for objc 
ivar 'barWithoutPrefix' [readability-identifier-naming]
+// CHECK-FIXES: int _barWithoutPrefix;
+}
+@end
Index: clang-tidy/readability/IdentifierNamingCheck.cpp
===
--- clang-tidy/readability/IdentifierNamingCheck.cpp
+++ clang-tidy/readability/IdentifierNamingCheck.cpp
@@ -109,6 +109,7 @@
 m(TemplateParameter) \
 m(TypeAlias) \
 m(MacroDefinition) \
+m(ObjcIvar) \
 
 enum StyleKind {
 #define ENUMERATE(v) SK_ ## v,
@@ -384,6 +385,9 @@
 const NamedDecl *D,
 const std::vector
 ) {
+  if (isa(D) && NamingStyles[SK_ObjcIvar])
+return SK_ObjcIvar;
+
   if (isa(D) && NamingStyles[SK_Typedef])
 return SK_Typedef;
 


Index: test/clang-tidy/readability-identifier-naming-objc.m
===
--- /dev/null
+++ test/clang-tidy/readability-identifier-naming-objc.m
@@ -0,0 +1,15 @@
+// RUN: %check_clang_tidy %s readability-identifier-naming %t \
+// RUN: -config='{CheckOptions: \
+// RUN:  [{key: readability-identifier-naming.ObjcIvarPrefix, value: '_'}]}' \
+// RUN: --
+
+@interface Foo
+@end 
+
+@interface Foo () {
+int _bar;
+int barWithoutPrefix;
+// CHECK-MESSAGES: :[[@LINE-1]]:9: warning: invalid case style for objc ivar 'barWithoutPrefix' [readability-identifier-naming]
+// CHECK-FIXES: int _barWithoutPrefix;
+}
+@end
Index: clang-tidy/readability/IdentifierNamingCheck.cpp
===
--- clang-tidy/readability/IdentifierNamingCheck.cpp
+++ clang-tidy/readability/IdentifierNamingCheck.cpp
@@ -109,6 +109,7 @@
 m(TemplateParameter) \
 m(TypeAlias) \
 m(MacroDefinition) \
+m(ObjcIvar) \
 
 enum StyleKind {
 #define ENUMERATE(v) SK_ ## v,
@@ -384,6 +385,9 @@
 const NamedDecl *D,
 const std::vector
 ) {
+  if (isa(D) && NamingStyles[SK_ObjcIvar])
+return SK_ObjcIvar;
+
   if (isa(D) && NamingStyles[SK_Typedef])
 return SK_Typedef;
 
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D45392: [clang-tidy] add new check to find out objc ivars which do not have prefix '_'

[Alexander Kornienko via Phabricator via cfe-commits]

alexfh requested changes to this revision.
alexfh added a comment.
This revision now requires changes to proceed.

Thank you, that's much better!

I'd also appreciate, if you could document the options this check supports in 
its .rst document (separately from this patch).

A few more comments inline.




Comment at: test/clang-tidy/readability-identifier-naming-objc.m:1-3
+// Remove UNSUPPORTED for powerpc64le when the problem introduced by
+// r288563 is resolved.
+// UNSUPPORTED: powerpc64le

I'd try committing the patch without disabling the test for powerpc64le and see 
whether the relevant buildbot complains. Looking at the description of r288563, 
I don't see anything that could be relevant to this test.



Comment at: test/clang-tidy/readability-identifier-naming-objc.m:7
+// RUN: {key: readability-identifier-naming.ObjcIvarPrefix, value: '_'}, \
+// RUN:   ]}' -- -fno-delayed-template-parsing \
+// RUN:   -I%S/Inputs/readability-identifier-naming \

I suspect the -fno-delayed-template-parsing is not needed, since no templates 
are used in this test.



Comment at: test/clang-tidy/readability-identifier-naming-objc.m:8-9
+// RUN:   ]}' -- -fno-delayed-template-parsing \
+// RUN:   -I%S/Inputs/readability-identifier-naming \
+// RUN:   -isystem %S/Inputs/readability-identifier-naming/system
+

It looks like these flags are not needed, since there are no #include 
directives in this test.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D45392



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

[PATCH] D45392: [clang-tidy] add new check to find out objc ivars which do not have prefix '_'

[Eugene Zelenko via Phabricator via cfe-commits]

Eugene.Zelenko added a comment.

How about doing same for //objc-property-declaration//?


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D45392



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

[PATCH] D45392: [clang-tidy] add new check to find out objc ivars which do not have prefix '_'

[Yan Zhang via Phabricator via cfe-commits]

Wizard added a comment.

In https://reviews.llvm.org/D45392#1063164, @alexfh wrote:

> In https://reviews.llvm.org/D45392#1061960, @Wizard wrote:
>
> > In https://reviews.llvm.org/D45392#1061433, @alexfh wrote:
> >
> > > I wonder whether the readability-identifier-naming check could be 
> > > extended to support this use case instead of adding a new check 
> > > specifically for underscores in ivar names?
> >
> >
> > Hmm readability-identifier-naming is a C++ check but this one is only for 
> > ObjC. I prefer putting them in separate places unless they work for both 
> > languages.
>
>
> I see no reasons why this check can't work for ObjC, if the handling of 
> ObjC-specific AST nodes is added to it.
>
> > Moreover, readability-identifier-naming always runs all matchers and apply 
> > the same checks on all identifiers.
>
> It has some sort of a hierarchical structure of rules that allow it to only 
> touch a certain subset of identifiers. E.g. configure different naming styles 
> for local variables and local constants. What it's lacking is a good 
> documentation for all of these options =\
>
> > We have to change at least part of the structure to make it applicable for 
> > this check.
>
> Yes, the existing check may need some modifications to do what this check 
> needs to do, but I'd expect these modifications to be quite small, and we 
> would potentially get a much more useful and generic tool instead of "one 
> check per type of named entity per naming style" situation.
>
> > And readability-identifier-naming is not in google default clang-tidy check 
> > list yet. We will even need more work to import it.
>
> IIUC, it can be configured to only verify certain kinds of named entities. 
> Thus there's no requirement to make it work with every aspect of our naming 
> rules.
>
> > So I think creating a new simple ObjC-specific check is a better way here.
>
> I'll respectfully disagree here. I would prefer to have a generic solution, 
> if it's feasible. And so far, it looks like it is.


Done. Yes it is very small changes to integrate this into it. I like it. The 
only annoying part was the doc is not that good and I spent a lot of time 
understanding the whole structure. Just one question, when we enable it in 
google by default, do we need to take care of the flags in the tests like 
-I%S/Inputs/readability-identifier-naming and -isystem 
%S/Inputs/readability-identifier-naming/system?


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D45392



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

[PATCH] D45392: [clang-tidy] add new check to find out objc ivars which do not have prefix '_'

[Yan Zhang via Phabricator via cfe-commits]

Wizard updated this revision to Diff 141938.
Wizard added a comment.

move check to readability-identifier-naming


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D45392

Files:
  clang-tidy/readability/IdentifierNamingCheck.cpp
  test/clang-tidy/readability-identifier-naming-objc.m


Index: test/clang-tidy/readability-identifier-naming-objc.m
===
--- /dev/null
+++ test/clang-tidy/readability-identifier-naming-objc.m
@@ -0,0 +1,20 @@
+// Remove UNSUPPORTED for powerpc64le when the problem introduced by
+// r288563 is resolved.
+// UNSUPPORTED: powerpc64le
+// RUN: %check_clang_tidy %s readability-identifier-naming %t -- \
+// RUN:   -config='{CheckOptions: [ \
+// RUN: {key: readability-identifier-naming.ObjcIvarPrefix, value: '_'}, \
+// RUN:   ]}' -- -fno-delayed-template-parsing \
+// RUN:   -I%S/Inputs/readability-identifier-naming \
+// RUN:   -isystem %S/Inputs/readability-identifier-naming/system
+
+@interface Foo
+@end 
+
+@interface Foo () {
+int _bar;
+int barWithoutPrefix;
+// CHECK-MESSAGES: :[[@LINE-1]]:9: warning: invalid case style for objc 
ivar 'barWithoutPrefix' [readability-identifier-naming]
+// CHECK-FIXES: int _barWithoutPrefix;
+}
+@end
Index: clang-tidy/readability/IdentifierNamingCheck.cpp
===
--- clang-tidy/readability/IdentifierNamingCheck.cpp
+++ clang-tidy/readability/IdentifierNamingCheck.cpp
@@ -109,6 +109,7 @@
 m(TemplateParameter) \
 m(TypeAlias) \
 m(MacroDefinition) \
+m(ObjcIvar) \
 
 enum StyleKind {
 #define ENUMERATE(v) SK_ ## v,
@@ -384,6 +385,9 @@
 const NamedDecl *D,
 const std::vector
 ) {
+  if (isa(D) && NamingStyles[SK_ObjcIvar])
+return SK_ObjcIvar;
+
   if (isa(D) && NamingStyles[SK_Typedef])
 return SK_Typedef;
 


Index: test/clang-tidy/readability-identifier-naming-objc.m
===
--- /dev/null
+++ test/clang-tidy/readability-identifier-naming-objc.m
@@ -0,0 +1,20 @@
+// Remove UNSUPPORTED for powerpc64le when the problem introduced by
+// r288563 is resolved.
+// UNSUPPORTED: powerpc64le
+// RUN: %check_clang_tidy %s readability-identifier-naming %t -- \
+// RUN:   -config='{CheckOptions: [ \
+// RUN: {key: readability-identifier-naming.ObjcIvarPrefix, value: '_'}, \
+// RUN:   ]}' -- -fno-delayed-template-parsing \
+// RUN:   -I%S/Inputs/readability-identifier-naming \
+// RUN:   -isystem %S/Inputs/readability-identifier-naming/system
+
+@interface Foo
+@end 
+
+@interface Foo () {
+int _bar;
+int barWithoutPrefix;
+// CHECK-MESSAGES: :[[@LINE-1]]:9: warning: invalid case style for objc ivar 'barWithoutPrefix' [readability-identifier-naming]
+// CHECK-FIXES: int _barWithoutPrefix;
+}
+@end
Index: clang-tidy/readability/IdentifierNamingCheck.cpp
===
--- clang-tidy/readability/IdentifierNamingCheck.cpp
+++ clang-tidy/readability/IdentifierNamingCheck.cpp
@@ -109,6 +109,7 @@
 m(TemplateParameter) \
 m(TypeAlias) \
 m(MacroDefinition) \
+m(ObjcIvar) \
 
 enum StyleKind {
 #define ENUMERATE(v) SK_ ## v,
@@ -384,6 +385,9 @@
 const NamedDecl *D,
 const std::vector
 ) {
+  if (isa(D) && NamingStyles[SK_ObjcIvar])
+return SK_ObjcIvar;
+
   if (isa(D) && NamingStyles[SK_Typedef])
 return SK_Typedef;
 
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D45392: [clang-tidy] add new check to find out objc ivars which do not have prefix '_'

[Alexander Kornienko via Phabricator via cfe-commits]

alexfh added a comment.

In https://reviews.llvm.org/D45392#1061960, @Wizard wrote:

> In https://reviews.llvm.org/D45392#1061433, @alexfh wrote:
>
> > I wonder whether the readability-identifier-naming check could be extended 
> > to support this use case instead of adding a new check specifically for 
> > underscores in ivar names?
>
>
> Hmm readability-identifier-naming is a C++ check but this one is only for 
> ObjC. I prefer putting them in separate places unless they work for both 
> languages.


I see no reasons why this check can't work for ObjC, if the handling of 
ObjC-specific AST nodes is added to it.

> Moreover, readability-identifier-naming always runs all matchers and apply 
> the same checks on all identifiers.

It has some sort of a hierarchical structure of rules that allow it to only 
touch a certain subset of identifiers. E.g. configure different naming styles 
for local variables and local constants. What it's lacking is a good 
documentation for all of these options =\

> We have to change at least part of the structure to make it applicable for 
> this check.

Yes, the existing check may need some modifications to do what this check needs 
to do, but I'd expect these modifications to be quite small, and we would 
potentially get a much more useful and generic tool instead of "one check per 
type of named entity per naming style" situation.

> And readability-identifier-naming is not in google default clang-tidy check 
> list yet. We will even need more work to import it.

IIUC, it can be configured to only verify certain kinds of named entities. Thus 
there's no requirement to make it work with every aspect of our naming rules.

> So I think creating a new simple ObjC-specific check is a better way here.

I'll respectfully disagree here. I would prefer to have a generic solution, if 
it's feasible. And so far, it looks like it is.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D45392



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

[PATCH] D45392: [clang-tidy] add new check to find out objc ivars which do not have prefix '_'

[Yan Zhang via Phabricator via cfe-commits]

Wizard added a comment.

In https://reviews.llvm.org/D45392#1061433, @alexfh wrote:

> I wonder whether the readability-identifier-naming check could be extended to 
> support this use case instead of adding a new check specifically for 
> underscores in ivar names?


Hmm readability-identifier-naming is a C++ check but this one is only for ObjC. 
I prefer putting them in separate places unless they work for both languages.
Moreover, readability-identifier-naming always runs all matchers and apply the 
same checks on all identifiers. We have to change at least part of the 
structure to make it applicable for this check. And 
readability-identifier-naming is not in google default clang-tidy check list 
yet. We will even need more work to import it.
So I think creating a new simple ObjC-specific check is a better way here.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D45392



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

[PATCH] D45392: [clang-tidy] add new check to find out objc ivars which do not have prefix '_'

[Alexander Kornienko via Phabricator via cfe-commits]

alexfh added a comment.

I wonder whether the readability-identifier-naming check could be extended to 
support this use case instead of adding a new check specifically for 
underscores in ivar names?


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D45392



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

[PATCH] D45392: [clang-tidy] add new check to find out objc ivars which do not have prefix '_'

[Yan Zhang via Phabricator via cfe-commits]

Wizard updated this revision to Diff 141574.
Wizard added a comment.

reorder release note for alphabetical order


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D45392

Files:
  clang-tidy/objc/CMakeLists.txt
  clang-tidy/objc/IvarDeclarationCheck.cpp
  clang-tidy/objc/IvarDeclarationCheck.h
  clang-tidy/objc/ObjCTidyModule.cpp
  docs/ReleaseNotes.rst
  docs/clang-tidy/checks/list.rst
  docs/clang-tidy/checks/objc-ivar-declaration.rst
  test/clang-tidy/objc-ivar-declaration.m

Index: test/clang-tidy/objc-ivar-declaration.m
===
--- /dev/null
+++ test/clang-tidy/objc-ivar-declaration.m
@@ -0,0 +1,11 @@
+// RUN: %check_clang_tidy %s objc-ivar-declaration %t
+@interface Foo
+@end 
+
+@interface Foo () {
+int _bar;
+int barWithoutPrefix;
+// CHECK-MESSAGES: :[[@LINE-1]]:9: warning: instance variable 'barWithoutPrefix' not using '_' as prefix [objc-ivar-declaration]
+// CHECK-FIXES: int _barWithoutPrefix;
+}
+@end
Index: docs/clang-tidy/checks/objc-ivar-declaration.rst
===
--- /dev/null
+++ docs/clang-tidy/checks/objc-ivar-declaration.rst
@@ -0,0 +1,22 @@
+.. title:: clang-tidy - objc-ivar-declaration
+
+objc-ivar-declaration
+=
+
+Finds Objective-C ivars that do not have a '_' prefix.
+According to Apple's programming guide, the ivar names should be 
+prefixed with '_'.
+
+For code of ivar declaration:
+
+.. code-block:: objc
+
+   int barWithoutPrefix;
+
+The fix will be:
+
+.. code-block:: objc
+
+   int _barWithoutPrefix;
+
+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
@@ -187,6 +187,7 @@
objc-avoid-nserror-init
objc-avoid-spinlock
objc-forbidden-subclassing
+   objc-ivar-declaration
objc-property-declaration
performance-faster-string-find
performance-for-range-copy
Index: docs/ReleaseNotes.rst
===
--- docs/ReleaseNotes.rst
+++ docs/ReleaseNotes.rst
@@ -124,6 +124,11 @@
   Finds and replaces deprecated uses of ``std::uncaught_exception`` to
   ``std::uncaught_exceptions``.
 
+- New :doc:`objc-ivar-declaration
+  ` check
+
+  Finds Objective-C ivars that do not have a '_' prefix.
+
 - New :doc:`portability-simd-intrinsics
   ` check
 
Index: clang-tidy/objc/ObjCTidyModule.cpp
===
--- clang-tidy/objc/ObjCTidyModule.cpp
+++ clang-tidy/objc/ObjCTidyModule.cpp
@@ -13,6 +13,7 @@
 #include "AvoidNSErrorInitCheck.h"
 #include "AvoidSpinlockCheck.h"
 #include "ForbiddenSubclassingCheck.h"
+#include "IvarDeclarationCheck.h"
 #include "PropertyDeclarationCheck.h"
 
 using namespace clang::ast_matchers;
@@ -30,6 +31,8 @@
 "objc-avoid-spinlock");
 CheckFactories.registerCheck(
 "objc-forbidden-subclassing");
+CheckFactories.registerCheck(
+"objc-ivar-declaration");
 CheckFactories.registerCheck(
 "objc-property-declaration");
   }
Index: clang-tidy/objc/IvarDeclarationCheck.h
===
--- /dev/null
+++ clang-tidy/objc/IvarDeclarationCheck.h
@@ -0,0 +1,35 @@
+//===--- IvarDeclarationCheck.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_IVARDECLARATIONCHECK_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_OBJC_IVARDECLARATIONCHECK_H
+
+#include "../ClangTidy.h"
+
+namespace clang {
+namespace tidy {
+namespace objc {
+
+/// Finds Objective-C ivars which do not have '_' prefix.
+///
+/// For the user-facing documentation see:
+/// http://clang.llvm.org/extra/clang-tidy/checks/objc-ivar-declaration.html
+class IvarDeclarationCheck : public ClangTidyCheck {
+public:
+  IvarDeclarationCheck(StringRef Name, ClangTidyContext *Context)
+  : ClangTidyCheck(Name, Context) {}
+  void registerMatchers(ast_matchers::MatchFinder *Finder) override;
+  void check(const ast_matchers::MatchFinder::MatchResult ) override;
+};
+
+} // namespace objc
+} // namespace tidy
+} // namespace clang
+
+#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_OBJC_IVARDECLARATIONCHECK_H
Index: clang-tidy/objc/IvarDeclarationCheck.cpp
===
--- /dev/null
+++ clang-tidy/objc/IvarDeclarationCheck.cpp
@@ -0,0 +1,52 @@
+//===--- 

[PATCH] D45392: [clang-tidy] add new check to find out objc ivars which do not have prefix '_'

[Eugene Zelenko via Phabricator via cfe-commits]

Eugene.Zelenko added a comment.

In https://reviews.llvm.org/D45392#1061064, @Wizard wrote:

> In https://reviews.llvm.org/D45392#1060971, @Eugene.Zelenko wrote:
>
> > In https://reviews.llvm.org/D45392#1060912, @Wizard wrote:
> >
> > > In https://reviews.llvm.org/D45392#1060854, @Eugene.Zelenko wrote:
> > >
> > > > In https://reviews.llvm.org/D45392#1060845, @Wizard wrote:
> > > >
> > > > > In https://reviews.llvm.org/D45392#1060485, @Eugene.Zelenko wrote:
> > > > >
> > > > > > If this is Apple guideline, check name should reflect this. I think 
> > > > > > will be good idea to have general check for Apple naming 
> > > > > > conventions instead of separate checks for specific situations like 
> > > > > > //objc-ivar-declaration// and //objc-property-declaration//.
> > > > >
> > > > >
> > > > > Thanks for the suggestion. I understand your point that they are both 
> > > > > naming convention, however, they are about different components and 
> > > > > using totally different naming rules. PropertyDeclarationCheck is 
> > > > > already a very complicated check (the most complicated one for ObjC), 
> > > > > I would rather not make it more heavy and try my best to split 
> > > > > independent logic to different checks.
> > > >
> > > >
> > > > See readability-identifier-naming 
> > > > 
> > > >  as example of multiple rules in one check.
> > >
> > >
> > > I took a look at IdentifierNamingCheck. Here's my thought:
> > >
> > > 1. IdentifierNamingCheck is trying to apply configurable naming 
> > > convention to C++ identifiers, and all the identifiers will share the 
> > > same style set. That is not the case of ObjC, where we follow Apple's 
> > > programming guide, and different types of identifiers are using different 
> > > style.
> > > 2. Such pattern can handle complicated requirements but to me it is not 
> > > simple enough to read and maintain. I would rather keep things simple and 
> > > clear as long as we have choice.
> > >
> > >   However, this check provides a good example of refactoring if in the 
> > > future we have the needs of organizing complicated naming styles. Moving 
> > > from simplicity to complexity is always easier. Thanks for pointing this 
> > > out for us.
> >
> >
> > My point is not flexibility of configuration, but handling of various types 
> > of identifiers in same check, even if conventions are different.
>
>
> Yes I understand but I mean "flexibility of configuration" is one of the 
> reasons of handling of various types of identifiers in same check, but we 
> don't need it here.


From user point of view, it's much easy to have one check which will check all 
possible types of identifiers, then set of not so obviously related checks.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D45392



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

[PATCH] D45392: [clang-tidy] add new check to find out objc ivars which do not have prefix '_'

[Yan Zhang via Phabricator via cfe-commits]

Wizard added a comment.

In https://reviews.llvm.org/D45392#1060971, @Eugene.Zelenko wrote:

> In https://reviews.llvm.org/D45392#1060912, @Wizard wrote:
>
> > In https://reviews.llvm.org/D45392#1060854, @Eugene.Zelenko wrote:
> >
> > > In https://reviews.llvm.org/D45392#1060845, @Wizard wrote:
> > >
> > > > In https://reviews.llvm.org/D45392#1060485, @Eugene.Zelenko wrote:
> > > >
> > > > > If this is Apple guideline, check name should reflect this. I think 
> > > > > will be good idea to have general check for Apple naming conventions 
> > > > > instead of separate checks for specific situations like 
> > > > > //objc-ivar-declaration// and //objc-property-declaration//.
> > > >
> > > >
> > > > Thanks for the suggestion. I understand your point that they are both 
> > > > naming convention, however, they are about different components and 
> > > > using totally different naming rules. PropertyDeclarationCheck is 
> > > > already a very complicated check (the most complicated one for ObjC), I 
> > > > would rather not make it more heavy and try my best to split 
> > > > independent logic to different checks.
> > >
> > >
> > > See readability-identifier-naming 
> > > 
> > >  as example of multiple rules in one check.
> >
> >
> > I took a look at IdentifierNamingCheck. Here's my thought:
> >
> > 1. IdentifierNamingCheck is trying to apply configurable naming convention 
> > to C++ identifiers, and all the identifiers will share the same style set. 
> > That is not the case of ObjC, where we follow Apple's programming guide, 
> > and different types of identifiers are using different style.
> > 2. Such pattern can handle complicated requirements but to me it is not 
> > simple enough to read and maintain. I would rather keep things simple and 
> > clear as long as we have choice.
> >
> >   However, this check provides a good example of refactoring if in the 
> > future we have the needs of organizing complicated naming styles. Moving 
> > from simplicity to complexity is always easier. Thanks for pointing this 
> > out for us.
>
>
> My point is not flexibility of configuration, but handling of various types 
> of identifiers in same check, even if conventions are different.


Yes I understand but I mean "flexibility of configuration" is one of the 
reasons of handling of various types of identifiers in same check, but we don't 
need it here.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D45392



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

[PATCH] D45392: [clang-tidy] add new check to find out objc ivars which do not have prefix '_'

[Eugene Zelenko via Phabricator via cfe-commits]

Eugene.Zelenko added a comment.

In https://reviews.llvm.org/D45392#1060912, @Wizard wrote:

> In https://reviews.llvm.org/D45392#1060854, @Eugene.Zelenko wrote:
>
> > In https://reviews.llvm.org/D45392#1060845, @Wizard wrote:
> >
> > > In https://reviews.llvm.org/D45392#1060485, @Eugene.Zelenko wrote:
> > >
> > > > If this is Apple guideline, check name should reflect this. I think 
> > > > will be good idea to have general check for Apple naming conventions 
> > > > instead of separate checks for specific situations like 
> > > > //objc-ivar-declaration// and //objc-property-declaration//.
> > >
> > >
> > > Thanks for the suggestion. I understand your point that they are both 
> > > naming convention, however, they are about different components and using 
> > > totally different naming rules. PropertyDeclarationCheck is already a 
> > > very complicated check (the most complicated one for ObjC), I would 
> > > rather not make it more heavy and try my best to split independent logic 
> > > to different checks.
> >
> >
> > See readability-identifier-naming 
> > 
> >  as example of multiple rules in one check.
>
>
> I took a look at IdentifierNamingCheck. Here's my thought:
>
> 1. IdentifierNamingCheck is trying to apply configurable naming convention to 
> C++ identifiers, and all the identifiers will share the same style set. That 
> is not the case of ObjC, where we follow Apple's programming guide, and 
> different types of identifiers are using different style.
> 2. Such pattern can handle complicated requirements but to me it is not 
> simple enough to read and maintain. I would rather keep things simple and 
> clear as long as we have choice.
>
>   However, this check provides a good example of refactoring if in the future 
> we have the needs of organizing complicated naming styles. Moving from 
> simplicity to complexity is always easier. Thanks for pointing this out for 
> us.


My point is not flexibility of configuration, but handling of various types of 
identifiers in same check, even if conventions are different.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D45392



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

[PATCH] D45392: [clang-tidy] add new check to find out objc ivars which do not have prefix '_'

[Yan Zhang via Phabricator via cfe-commits]

Wizard added a comment.

In https://reviews.llvm.org/D45392#1060854, @Eugene.Zelenko wrote:

> In https://reviews.llvm.org/D45392#1060845, @Wizard wrote:
>
> > In https://reviews.llvm.org/D45392#1060485, @Eugene.Zelenko wrote:
> >
> > > If this is Apple guideline, check name should reflect this. I think will 
> > > be good idea to have general check for Apple naming conventions instead 
> > > of separate checks for specific situations like //objc-ivar-declaration// 
> > > and //objc-property-declaration//.
> >
> >
> > Thanks for the suggestion. I understand your point that they are both 
> > naming convention, however, they are about different components and using 
> > totally different naming rules. PropertyDeclarationCheck is already a very 
> > complicated check (the most complicated one for ObjC), I would rather not 
> > make it more heavy and try my best to split independent logic to different 
> > checks.
>
>
> See readability-identifier-naming 
> 
>  as example of multiple rules in one check.


I took a look at IdentifierNamingCheck. Here's my thought:

1. IdentifierNamingCheck is trying to apply configurable naming convention to 
C++ identifiers, and all the identifiers will share the same style set. That is 
not the case of ObjC, where we follow Apple's programming guide, and different 
types of identifiers are using different style.
2. Such pattern can handle complicated requirements but to me it is not simple 
enough to read and maintain. I would rather keep things simple and clear as 
long as we have choice.

However, this check provides a good example of refactoring if in the future we 
have the needs of organizing complicated naming styles. Moving from simplicity 
to complexity is always easier. Thanks for pointing this out for us.




Comment at: docs/ReleaseNotes.rst:60
 
+- New :doc:`objc-ivar-declaration
+  ` check

Eugene.Zelenko wrote:
> Wizard wrote:
> > Eugene.Zelenko wrote:
> > > Please place in new check list in alphabetical order.
> > I did not see a "new check list" in alphabetical order in this file. I 
> > believe they are ordered by commit time.If you mean the list.rst, they are 
> > already ordered alphabetically.
> Please read starting words in list of changes.
Hmm line 96 is "- New :doc:`fuchsia-multiple-inheritance 
` check", while line 101 is "- 
New :doc:`abseil-string-find-startswith 
` check". And similar thing 
happened to line 138. I thought it wasn't alphabetical. But that's probably 
mistakes from others. I will fix this one.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D45392



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

[PATCH] D45392: [clang-tidy] add new check to find out objc ivars which do not have prefix '_'

[Eugene Zelenko via Phabricator via cfe-commits]

Eugene.Zelenko added a comment.

In https://reviews.llvm.org/D45392#1060845, @Wizard wrote:

> In https://reviews.llvm.org/D45392#1060485, @Eugene.Zelenko wrote:
>
> > If this is Apple guideline, check name should reflect this. I think will be 
> > good idea to have general check for Apple naming conventions instead of 
> > separate checks for specific situations like //objc-ivar-declaration// and 
> > //objc-property-declaration//.
>
>
> Thanks for the suggestion. I understand your point that they are both naming 
> convention, however, they are about different components and using totally 
> different naming rules. PropertyDeclarationCheck is already a very 
> complicated check (the most complicated one for ObjC), I would rather not 
> make it more heavy and try my best to split independent logic to different 
> checks.


See readability-identifier-naming 

 as example of multiple rules in one check.




Comment at: docs/ReleaseNotes.rst:60
 
+- New :doc:`objc-ivar-declaration
+  ` check

Wizard wrote:
> Eugene.Zelenko wrote:
> > Please place in new check list in alphabetical order.
> I did not see a "new check list" in alphabetical order in this file. I 
> believe they are ordered by commit time.If you mean the list.rst, they are 
> already ordered alphabetically.
Please read starting words in list of changes.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D45392



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

[PATCH] D45392: [clang-tidy] add new check to find out objc ivars which do not have prefix '_'

[Yan Zhang via Phabricator via cfe-commits]

Wizard updated this revision to Diff 141515.
Wizard edited the summary of this revision.
Wizard added a comment.

resolve comments


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D45392

Files:
  clang-tidy/objc/CMakeLists.txt
  clang-tidy/objc/IvarDeclarationCheck.cpp
  clang-tidy/objc/IvarDeclarationCheck.h
  clang-tidy/objc/ObjCTidyModule.cpp
  docs/ReleaseNotes.rst
  docs/clang-tidy/checks/list.rst
  docs/clang-tidy/checks/objc-ivar-declaration.rst
  test/clang-tidy/objc-ivar-declaration.m

Index: test/clang-tidy/objc-ivar-declaration.m
===
--- /dev/null
+++ test/clang-tidy/objc-ivar-declaration.m
@@ -0,0 +1,11 @@
+// RUN: %check_clang_tidy %s objc-ivar-declaration %t
+@interface Foo
+@end 
+
+@interface Foo () {
+int _bar;
+int barWithoutPrefix;
+// CHECK-MESSAGES: :[[@LINE-1]]:9: warning: instance variable 'barWithoutPrefix' not using '_' as prefix [objc-ivar-declaration]
+// CHECK-FIXES: int _barWithoutPrefix;
+}
+@end
Index: docs/clang-tidy/checks/objc-ivar-declaration.rst
===
--- /dev/null
+++ docs/clang-tidy/checks/objc-ivar-declaration.rst
@@ -0,0 +1,22 @@
+.. title:: clang-tidy - objc-ivar-declaration
+
+objc-ivar-declaration
+=
+
+Finds Objective-C ivars that do not have a '_' prefix.
+According to Apple's programming guide, the ivar names should be 
+prefixed with '_'.
+
+For code of ivar declaration:
+
+.. code-block:: objc
+
+   int barWithoutPrefix;
+
+The fix will be:
+
+.. code-block:: objc
+
+   int _barWithoutPrefix;
+
+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
@@ -187,6 +187,7 @@
objc-avoid-nserror-init
objc-avoid-spinlock
objc-forbidden-subclassing
+   objc-ivar-declaration
objc-property-declaration
performance-faster-string-find
performance-for-range-copy
Index: docs/ReleaseNotes.rst
===
--- docs/ReleaseNotes.rst
+++ docs/ReleaseNotes.rst
@@ -57,6 +57,11 @@
 Improvements to clang-tidy
 --
 
+- New :doc:`objc-ivar-declaration
+  ` check
+
+  Finds Objective-C ivars that do not have a '_' prefix.
+
 - New module `abseil` for checks related to the `Abseil `_
   library.
 
Index: clang-tidy/objc/ObjCTidyModule.cpp
===
--- clang-tidy/objc/ObjCTidyModule.cpp
+++ clang-tidy/objc/ObjCTidyModule.cpp
@@ -13,6 +13,7 @@
 #include "AvoidNSErrorInitCheck.h"
 #include "AvoidSpinlockCheck.h"
 #include "ForbiddenSubclassingCheck.h"
+#include "IvarDeclarationCheck.h"
 #include "PropertyDeclarationCheck.h"
 
 using namespace clang::ast_matchers;
@@ -30,6 +31,8 @@
 "objc-avoid-spinlock");
 CheckFactories.registerCheck(
 "objc-forbidden-subclassing");
+CheckFactories.registerCheck(
+"objc-ivar-declaration");
 CheckFactories.registerCheck(
 "objc-property-declaration");
   }
Index: clang-tidy/objc/IvarDeclarationCheck.h
===
--- /dev/null
+++ clang-tidy/objc/IvarDeclarationCheck.h
@@ -0,0 +1,35 @@
+//===--- IvarDeclarationCheck.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_IVARDECLARATIONCHECK_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_OBJC_IVARDECLARATIONCHECK_H
+
+#include "../ClangTidy.h"
+
+namespace clang {
+namespace tidy {
+namespace objc {
+
+/// Finds Objective-C ivars which do not have '_' prefix.
+///
+/// For the user-facing documentation see:
+/// http://clang.llvm.org/extra/clang-tidy/checks/objc-ivar-declaration.html
+class IvarDeclarationCheck : public ClangTidyCheck {
+public:
+  IvarDeclarationCheck(StringRef Name, ClangTidyContext *Context)
+  : ClangTidyCheck(Name, Context) {}
+  void registerMatchers(ast_matchers::MatchFinder *Finder) override;
+  void check(const ast_matchers::MatchFinder::MatchResult ) override;
+};
+
+} // namespace objc
+} // namespace tidy
+} // namespace clang
+
+#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_OBJC_IVARDECLARATIONCHECK_H
Index: clang-tidy/objc/IvarDeclarationCheck.cpp
===
--- /dev/null
+++ clang-tidy/objc/IvarDeclarationCheck.cpp
@@ -0,0 +1,52 @@
+//===--- 

[PATCH] D45392: [clang-tidy] add new check to find out objc ivars which do not have prefix '_'

[Yan Zhang via Phabricator via cfe-commits]

Wizard marked 4 inline comments as done.
Wizard added a comment.

In https://reviews.llvm.org/D45392#1060485, @Eugene.Zelenko wrote:

> If this is Apple guideline, check name should reflect this. I think will be 
> good idea to have general check for Apple naming conventions instead of 
> separate checks for specific situations like //objc-ivar-declaration// and 
> //objc-property-declaration//.


Thanks for the suggestion. I understand your point that they are both naming 
convention, however, they are about different components and using totally 
different naming rules. PropertyDeclarationCheck is already a very complicated 
check (the most complicated one for ObjC), I would rather not make it more 
heavy and try my best to split independent logic to different checks.




Comment at: clang-tidy/objc/IvarDeclarationCheck.cpp:23
+
+FixItHint generateFixItHint(const ObjCIvarDecl *Decl) {
+  auto IvarName = Decl->getName();

Eugene.Zelenko wrote:
> Please use static instead of anonymous namespace.
Using anonymous namespace was suggested by others for private methods that are 
only used within the check (e.g. ForbiddenSubclassingCheck, 
PropertyDeclarationCheck, etc). I would rather keep this pattern consistent 
with other checks.



Comment at: docs/ReleaseNotes.rst:60
 
+- New :doc:`objc-ivar-declaration
+  ` check

Eugene.Zelenko wrote:
> Please place in new check list in alphabetical order.
I did not see a "new check list" in alphabetical order in this file. I believe 
they are ordered by commit time.If you mean the list.rst, they are already 
ordered alphabetically.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D45392



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

[PATCH] D45392: [clang-tidy] add new check to find out objc ivars which do not have prefix '_'

[Eugene Zelenko via Phabricator via cfe-commits]

Eugene.Zelenko added a comment.

If this is Apple guideline, check name should reflect this. I think will be 
good idea to have general check for Apple naming conventions instead of 
separate checks for specific situations like //objc-ivar-declaration// and 
//objc-property-declaration//.




Comment at: clang-tidy/objc/IvarDeclarationCheck.cpp:23
+
+FixItHint generateFixItHint(const ObjCIvarDecl *Decl) {
+  auto IvarName = Decl->getName();

Please use static instead of anonymous namespace.



Comment at: clang-tidy/objc/IvarDeclarationCheck.cpp:24
+FixItHint generateFixItHint(const ObjCIvarDecl *Decl) {
+  auto IvarName = Decl->getName();
+  if (IvarName[0] != '_') {

Please don't use auto when type could not be deduced from statement. Same in 
other places.



Comment at: docs/ReleaseNotes.rst:60
 
+- New :doc:`objc-ivar-declaration
+  ` check

Please place in new check list in alphabetical order.



Comment at: docs/ReleaseNotes.rst:63
+
+  New check that finds Objective-C ivars that do not have a '_' prefix.
+

Please remove //New check that//.



Comment at: docs/clang-tidy/checks/objc-ivar-declaration.rst:6
+
+Finds ivar declarations in Objective-C files that do not follow the pattern
+in Apple's programming guide. The ivar name should be fixed with '_'.

Please synchronize first statement with Release Notes.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D45392



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