[PATCH] D65917: [clang-tidy] Added check for the Google style guide's category method naming rule.

[David Gatwood via Phabricator via cfe-commits]

dgatwood added inline comments.


================
Comment at: clang-tools-extra/clang-tidy/google/RequireCategoryMethodPrefixesCheck.cpp:23
+  Finder->addMatcher(objcMethodDecl(hasDeclContext(
+      objcCategoryDecl())).bind(CustomCategoryMethodIdentifier), this);
+}
----------------
stephanemoore wrote:
> 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](https://clang.llvm.org/doxygen/classclang_1_1ObjCCategoryDecl.html#acdb14eeca277cfa745a4e8e842312008) and then add `isExpansionInSystemHeader` as an inner matcher on the custom inner matcher. WDYT? Let me know if you need help putting together.
Requiring users to specify which classes should be covered by this checker doesn't scale well.  System classes are a tiny fraction of the shared code that we bring in.  Proto classes alone probably outnumber system framework classes 10:1, plus all the shared code from other internal framework teams.  A list of every shared class that we bring in would be massive, and generating it programmatically would be relatively expensive.  The same problem exists for a list of prefixes to protect.

Also with that approach, a mistake by a person or script that maintains such a list would result in **not** getting warnings.  Silent failures are the worst kind of failure, because you don't even know that something is going wrong.  By contrast, if you require the user to specify a list of prefixes to ignore, as this patch does, then any mistake by someone maintaining the lest results in getting **extra** warnings, which makes it obvious that something is wrong and needs to be fixed.

I realize that ostensibly a team could own some code that is also shared, and it could have the same prefix as their app.  But there's no real reason to care about that case.  After all, if someone changes the shared code and it breaks the category, it's the same team, so their tests should catch the breakage, unlike changes made by some far-flung team on the other side of the world.  Also, if they break their own code, they also have permission to fix that breakage without additional approvals.  So that edge case is largely academic; if anybody asks for a way to not ignore specific classes that have an exempt prefix, we can certainly add that feature later pretty easily, but I really doubt anybody would bother to use it.  :-)


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D65917/new/

https://reviews.llvm.org/D65917