Page MenuHomePhabricator

[clang-tidy] Added check for the Google style guide's category method naming rule.
Needs RevisionPublic

Authored by dgatwood on Aug 7 2019, 6:25 PM.

Diff Detail

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
Herald added a project: Restricted Project. · View Herald TranscriptAug 7 2019, 6:25 PM
Eugene.Zelenko added inline comments.
clang-tools-extra/clang-tidy/google/RequireCategoryMethodPrefixesCheck.cpp
17

Please use static. See LLVM Coding Guidelines.

59

Please don't use auto, unless type presents in same statement or in case of iterators.

clang-tools-extra/docs/ReleaseNotes.rst
74

Please enclose gmo_methodName in double back-ticks.

clang-tools-extra/docs/clang-tidy/checks/google-objc-require-category-method-prefixes.rst
7

Please make first statement same as in Release Notes.

17

Please enclose gmo_ in double back-ticks.

20

See other checks documentation for proper option section style.

clang -> :program:clang-tidy and enclose WhitelistedPrefixes in single back-ticks.

25

Please enclose NSObject in double back-ticks.

32

Please enclose QED in double back-ticks.

Eugene.Zelenko retitled this revision from Added clang-tidy module for the Google style guide's category method naming rule. to [clang-tidy] Added check for the Google style guide's category method naming rule..Aug 7 2019, 10:06 PM
Eugene.Zelenko added a project: Restricted Project.
aaron.ballman added inline comments.Aug 8 2019, 12:53 PM
clang-tools-extra/clang-tidy/google/GoogleTidyModule.cpp
44

Please keep this list in alphabetical order.

clang-tools-extra/clang-tidy/google/RequireCategoryMethodPrefixesCheck.cpp
22

You should elide the braces here per our usual coding conventions (also applies elsewhere).

26

No need for the local here, I'd just inline it below.

38

I think this interface should accept a StringRef argument and either return a std::string or an llvm::Optional<std::string> rather than allocating a pointer.

44

No need for messy C APIs here -- you can compare a StringRef to a std::string directly.

58

This should use getName() to get a StringRef to avoid the copy.

63

Can use getName() and assign to a StringRef

101

Drop the period at the end of the diagnostic -- clang-tidy diagnostics are not meant to be grammatically correct.

102

You should pass method_declaration instead; it will get quoted and converted to the proper identifier automatically.

clang-tools-extra/clang-tidy/google/RequireCategoryMethodPrefixesCheck.h
29

You should pick names that match the LLVM coding style naming rules (here and elsewhere in the patch).

Whoops. I was expecting to get emailed about review comments and never got anything, so I thought it was just not getting reviewed. I'll work on this again. Sorry about that. :-)

dgatwood updated this revision to Diff 218478.Tue, Sep 3, 10:02 AM

Incorporated feedback.

Eugene.Zelenko added inline comments.Tue, Sep 3, 10:04 AM
clang-tools-extra/docs/ReleaseNotes.rst
70

Please sort new checks list alphabetically.

dgatwood marked 17 inline comments as done.Tue, Sep 3, 10:16 AM
dgatwood added inline comments.
clang-tools-extra/clang-tidy/google/RequireCategoryMethodPrefixesCheck.cpp
58

That's actually what I originally tried, but that method won't work here, unless I'm missing something. The getName() method crashes with a message saying that "Name is not a simple identifier".

clang-tools-extra/docs/clang-tidy/checks/google-objc-require-category-method-prefixes.rst
20

Removed the text in question, because it duplicates the Options info.

aaron.ballman added inline comments.Wed, Sep 4, 2:40 PM
clang-tools-extra/clang-tidy/google/RequireCategoryMethodPrefixesCheck.cpp
17

This comment also meant that you should drop the anonymous namespace (per our coding conventions -- we only use anonymous namespaces for types).

20

Finder

31

ExpectedPrefixes here as well.

Should there be a default list of these?

40–44

llvm::find_if(PrefixArray, [ClassName](const std::string &Str) { return ClassName.startswith(Str); });

50

MethodDeclaration

57

MethodName (and so on, I'll stop commenting on them.)

58

You can call getIdentifier() instead, and if you get a non-null object back, you can call getName() on that. If it is null, there's nothing to check.

58

Drop the clang::

65

Can use const auto * because the type is in the initialization. However, drop the clang:: as it's not needed.

76

I notice we never actually care about the string contained within MatchingPrefix, which suggests that matchingWhitelistedPrefix() could just return a bool.

clang-tools-extra/clang-tidy/google/RequireCategoryMethodPrefixesCheck.h
23–24

Finder and Result per coding style.

29–32

I think the name ExpectedPrefixes and matchesExpectedPrefix() are more clear.

32

class_name should be ClassName.

clang-tools-extra/docs/clang-tidy/checks/google-objc-require-category-method-prefixes.rst
5

Underlining looks off here.

dgatwood updated this revision to Diff 218812.Wed, Sep 4, 4:51 PM
dgatwood marked 22 inline comments as done.

Incorporated additional feedback.

aaron.ballman added inline comments.Thu, Sep 5, 4:59 AM
clang-tools-extra/clang-tidy/google/RequireCategoryMethodPrefixesCheck.cpp
16

Drop the k prefix (we don't use Hungarian notation).

31

Still wondering whether we should have a default list of expected prefixes or not.

37–41

Sorry for not recognizing this earlier, but since we don't care about which item was found, we can go with:

return llvm::any_of(PrefixArray, ...);
58

The comment to use getIdentifier() was marked as done but the changes were not applied; was that a mistake? I'm pushing back on getNameAsString() because the function is commented as having its use discouraged, so we should not be adding new uses of it.

clang-tools-extra/clang-tidy/google/RequireCategoryMethodPrefixesCheck.h
23–24

finder -> Finder
result -> Result

32

class_name -> ClassName

dgatwood updated this revision to Diff 218953.Thu, Sep 5, 10:40 AM
dgatwood marked 7 inline comments as done.

Fixed a couple of variable names in the .h file, renamed kCustomCategoryMethodIdentifier to CustomCategoryMethodIdentifier, and switched to llvm::any_of.

Ah. I expected my comments to be submitted as part of uploading a new revision. Still learning this UI. :-/

clang-tools-extra/clang-tidy/google/RequireCategoryMethodPrefixesCheck.cpp
31

Done. And no, there should be no default, unless somehow Xcode's project prefix makes it down as far as LLVM, in which case maybe that could be the default.

The idea is that you can whitelist your own Xcode project's prefix, along with the prefixes of your own in-house libraries, so that each individual team/workgroup can add categories on their own classes, but will get warned when they try to add unprefixed category methods on classes that they don't own (e.g. classes in system frameworks, third-party frameworks, etc.).

31

This is weird. I don't know why this comment system didn't submit my comment before.

No, there should be no default, unless somehow Xcode's project prefix makes it down as far as LLVM, in which case maybe that could be the default.

The idea is that you can whitelist your own Xcode project's prefix, along with the prefixes of your own in-house libraries, so that each individual team/workgroup can add categories on their own classes, but will get warned when they try to add unprefixed category methods on classes that they don't own (e.g. classes in system frameworks, third-party frameworks, etc.).

57

Yeah. I finally realized I should just grep for '_' instead of looking for them by hand — one of the joys of converting something originally written under Google's style guide so that it conforms to the llvm style guide instead. Sorry about that. I hope I got all of them this time.

58

I just tried it, and getIdentifier() returns NULL consistently for every category method, so I changed it back to getNameAsString(), which works.

58

I marked that as done because I tried it and it didn't work. The getIdentifier() method returned NULL for every category method.

BTW, this isn't my first attempt at writing this code in a way that doesn't require that method. I literally fought with getting the name of category methods for a day or more when I first started writing this, because I kept getting NULLs or crashes. At one point, I think I even tried looking for the owning class and querying its interface. Nothing worked until I discovered getNameAsString().

I'm assuming that this is simply a bug somewhere in the LLVM core that nobody has noticed or bothered to fix, because it really should not be difficult to get the name of a method. :-/

76

We could. Originally, I was printing it for diagnostics, but now that I ripped out the debug logging, it isn't really needed. Done.

clang-tools-extra/clang-tidy/google/RequireCategoryMethodPrefixesCheck.h
23–24

What the heck? Oh, I fixed it in the .cc file and not the .h file. Sorry. Done.

aaron.ballman added inline comments.Fri, Sep 6, 5:21 AM
clang-tools-extra/clang-tidy/google/RequireCategoryMethodPrefixesCheck.cpp
31

Ah, thank you for the explanation!

57

No worries, things are looking good now!

58

I'm assuming that this is simply a bug somewhere in the LLVM core that nobody has noticed or bothered to fix, because it really should not be difficult to get the name of a method. :-/

I am not certain if it's a bug or not, but we shouldn't be using essentially deprecated APIs to work around bugs elsewhere. I'd really like to know what's going on here. I am looking through the logic of DeclarationName::print() (which is what getNameAsString() eventually calls into) and it doesn't look like it makes any special accommodations for ObjC method declarations, but it does for ObjC selectors. I'm not an ObjC expert, but I think that's what the name of an ObjC method is, isn't it? If so, I think you would do (assuming the methods can only be named using selectors):

MethodDeclaration->getDeclName().getObjCSelector().getAsString()
clang-tools-extra/clang-tidy/google/RequireCategoryMethodPrefixesCheck.h
23–24

No worries!

clang-tools-extra/docs/clang-tidy/checks/google-objc-require-category-method-prefixes.rst
29

whitelist the QED three-letter prefix -> expect the three-letter prefix QED

dgatwood updated this revision to Diff 219151.Fri, Sep 6, 11:59 AM
dgatwood marked 9 inline comments as done.

Tweaked documentation, and replaced the one remaining instance of getNameAsString().

clang-tools-extra/clang-tidy/google/RequireCategoryMethodPrefixesCheck.cpp
58

Yeah, any Objective-C method inside a class is inherently named by a selector. That code works. Thanks. :-)

clang-tools-extra/docs/clang-tidy/checks/google-objc-require-category-method-prefixes.rst
29

Applied approximately. :-)

This revision is now accepted and ready to land.Sat, Sep 7, 1:14 PM
stephanemoore requested changes to this revision.Wed, Sep 11, 4:52 AM
stephanemoore marked an inline comment as done.
stephanemoore added inline comments.
clang-tools-extra/clang-tidy/google/RequireCategoryMethodPrefixesCheck.cpp
23–24

What do you think of objcMethodDecl(hasDeclContext(objcCategoryDecl()))? I think that more narrowly targets the method declarations that we are interested in. There are other method declarations that we like to prefix but I consider those outside the domain of category method name prefixing practices.

31

Agreed that there should be no default.

This revision now requires changes to proceed.Wed, Sep 11, 4:52 AM
dgatwood updated this revision to Diff 220123.Fri, Sep 13, 9:20 AM

Switched to objcMethodDecl(hasDeclContext(objcCategoryDecl())).

dgatwood marked an inline comment as done.Fri, Sep 13, 9:21 AM
stephanemoore requested changes to this revision.Mon, Sep 16, 7:02 PM
stephanemoore added inline comments.
clang-tools-extra/clang-tidy/google/RequireCategoryMethodPrefixesCheck.cpp
23

Technically category method prefixing is only strictly enforced on classes that are shared:
"If a class is not shared with other projects, categories extending it may omit name prefixes and method name prefixes."
https://github.com/google/styleguide/blob/gh-pages/objcguide.md#category-naming

With that in mind, perhaps we should match on categories on classes that extend classes that are declared in system headers? I think you can accomplish that by adding a custom inner matcher in objcCategoryDecl which pulls out the Objective-C interface using clang::ObjCCategoryDecl::getClassInterface and then add isExpansionInSystemHeader as an inner matcher on the custom inner matcher. WDYT? Let me know if you need help putting together.

clang-tools-extra/docs/clang-tidy/checks/google-objc-require-category-method-prefixes.rst
41

This option seems to describe a list of class prefixes that are whitelisted. If that is the case, perhaps WhitelistedPrefixes or WhitelistedClassPrefixes would be a better name for the option? WDYT?

This revision now requires changes to proceed.Mon, Sep 16, 7:02 PM
aaron.ballman added inline comments.Tue, Sep 17, 4:34 AM
clang-tools-extra/docs/clang-tidy/checks/google-objc-require-category-method-prefixes.rst
41

No, please. We should try to avoid "white" and "black" lists in favor of more descriptive terms with less social connotations. I'd be fine with ExpectedClassPrefixes, AllowedPrefixes, KnownPrefixes, etc.

alexfh removed a reviewer: alexfh.Tue, Sep 17, 5:32 AM
stephanemoore added inline comments.Tue, Sep 17, 1:48 PM
clang-tools-extra/docs/clang-tidy/checks/google-objc-require-category-method-prefixes.rst
41

Makes sense. Would ExemptClassPrefixes work?

aaron.ballman added inline comments.Wed, Sep 18, 5:05 AM
clang-tools-extra/docs/clang-tidy/checks/google-objc-require-category-method-prefixes.rst
41

I think that'd be a great name.