This is an archive of the discontinued LLVM Phabricator instance.

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

Authored by Wizard on Apr 6 2018, 3:38 PM.

Event Timeline

Wizard created this revision.Apr 6 2018, 3:38 PM
Wizard updated this revision to Diff 141444.Apr 6 2018, 3:41 PM

fix doc

Wizard edited the summary of this revision. (Show Details)Apr 6 2018, 3:42 PM
Wizard added reviewers: benhamilton, hokein.
Eugene.Zelenko retitled this revision from add new check to find out objc ivars which do not have prefix '_' to [clang-tidy] add new check to find out objc ivars which do not have prefix '_'.Apr 6 2018, 5:16 PM
Eugene.Zelenko added a project: Restricted Project.

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.

clang-tidy/objc/IvarDeclarationCheck.cpp
23

Please use static instead of anonymous namespace.

24

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

docs/ReleaseNotes.rst
60

Please place in new check list in alphabetical order.

63

Please remove New check that.

docs/clang-tidy/checks/objc-ivar-declaration.rst
6

Please synchronize first statement with Release Notes.

Wizard marked 4 inline comments as done.Apr 7 2018, 7:43 PM

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.

clang-tidy/objc/IvarDeclarationCheck.cpp
23

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.

docs/ReleaseNotes.rst
60

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.

Wizard marked 4 inline comments as done.Apr 7 2018, 7:44 PM
Wizard updated this revision to Diff 141515.Apr 7 2018, 7:45 PM
Wizard edited the summary of this revision. (Show Details)

resolve comments

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.

docs/ReleaseNotes.rst
60

Please read starting words in list of changes.

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.

docs/ReleaseNotes.rst
60

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.

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.

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.

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.

Wizard updated this revision to Diff 141574.Apr 8 2018, 4:09 PM

reorder release note for alphabetical order

alexfh added a comment.Apr 9 2018, 4:32 AM

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?

alexfh requested changes to this revision.Apr 9 2018, 4:32 AM
This revision now requires changes to proceed.Apr 9 2018, 4:32 AM

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.

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.

Wizard updated this revision to Diff 141938.Apr 10 2018, 6:04 PM

move check to readability-identifier-naming

How about doing same for objc-property-declaration?

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?

alexfh requested changes to this revision.Apr 11 2018, 6:37 AM

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.

test/clang-tidy/readability-identifier-naming-objc.m
1–3 ↗(On Diff #141938)

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.

7 ↗(On Diff #141938)

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

8–9 ↗(On Diff #141938)

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

This revision now requires changes to proceed.Apr 11 2018, 6:37 AM
Wizard updated this revision to Diff 142057.Apr 11 2018, 11:58 AM

remove unnecessary flags

Wizard marked 3 inline comments as done.Apr 11 2018, 11:59 AM
Wizard added inline comments.
test/clang-tidy/readability-identifier-naming-objc.m
8–9 ↗(On Diff #141938)

So we won't worry about those flags when using this check in google codebase right?

alexfh added inline comments.Apr 12 2018, 4:07 AM
test/clang-tidy/readability-identifier-naming-objc.m
4 ↗(On Diff #142057)

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

8–9 ↗(On Diff #141938)

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.

alexfh requested changes to this revision.Apr 12 2018, 6:37 AM
This revision now requires changes to proceed.Apr 12 2018, 6:37 AM

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

Wizard added inline comments.Apr 12 2018, 11:10 AM
test/clang-tidy/readability-identifier-naming-objc.m
4 ↗(On Diff #142057)

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: [:+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

Wizard marked an inline comment as done.Apr 12 2018, 11:10 AM
Wizard marked an inline comment as done.
alexfh accepted this revision.Apr 20 2018, 12:20 AM

LG. Thanks!

This revision is now accepted and ready to land.Apr 20 2018, 12:20 AM
This revision was automatically updated to reflect the committed changes.