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 
> <http://clang.llvm.org/extra/clang-tidy/checks/readability-identifier-naming.html>
>  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
+  <clang-tidy/checks/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 
<clang-tidy/checks/fuchsia-multiple-inheritance>` check", while line 101 is "- 
New :doc:`abseil-string-find-startswith 
<clang-tidy/checks/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.

  rCTE Clang Tools Extra


cfe-commits mailing list

Reply via email to