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

[Stephane Moore via Phabricator via cfe-commits]

stephanemoore added a comment.

(sorry for the delay 😅)

> The difference is that the exemption list would be per-project, created by whoever turns on the check for the project. I was assuming that this checker would be turned on by each team, rather than globally, and that part of doing so would be specifying which class prefixes should be ignored for that particular project.

The vision that I had in mind for this check was for it to be enabled more widely which would require it to be useful by default without requiring options to be specified to reduce noise.

> That said, if you want to turn this check on globally, we probably should support either approach, with the default being to check only categories on system frameworks and classes derived from a specific set of opted-in base classes. Then, if a project owner provides a set of prefixes to exempt, it would expand the check to cover everything not exempted. (That's the mode in which we would prefer to use the feature for our team's project.)
> 
> Either way, it is a good idea to auto-exempt any class that isn’t prefixed.

That's a good idea but note that detecting that a class name does not have a prefix is not easy because acronyms and initialisms are capitalized (e.g., a class named `URLCache` probably does not have a prefix).

> So basically, I'm thinking:
> 
> If no prefix list is provided:
> 
> Categories on:
> 
> System framework classes.
>  A specific list of protected classes and subclasses thereof.
>  If a prefix list is provided:
> 
> Categories on all classes EXCEPT:
> 
> Classes whose prefixes are explicitly excepted.
>  Classes that do not start with at least three capital letters (non-prefixed classes).
>  Does that approach seem reasonable to you?

I think we are moving in a good direction. I suspect that a single option may not be the best to control the relevant behaviors.

It sounds like we want the following behaviors:
(1) Require method prefixes on categories on system classes.
(2) Require method prefixes on categories on user classes //except// (2A) user classes whose names begin with an exempted prefix and (2B) user classes that are derived from an exempted base class.

I think it's unlikely that we would want (1) to be conditional but I think the others are likely options that developers would want to control. Based on that judgment call, I would imagine three options like so (option names have not been thought through extensively): `IncludeUserClasses`, `ExemptUserClassPrefixes`, and `ExemptUserBaseClasses`. That way when the check is enabled without any options the developer gets enforcement on system classes which should reduce a common source of errors and developers that want enforcement on their own classes can opt in and use the additional options to control enforcement and reduce noise.

WDYT?



================
Comment at: clang-tools-extra/docs/clang-tidy/checks/list.rst:233
    google-objc-global-variable-declaration
+   google-objc-require-category-method-prefixes
    google-readability-avoid-underscore-in-googletest-name
----------------
Technically this check is not specific to Google Objective-C:
"In order to avoid undefined behavior, it’s best practice to add a prefix to method names in categories on framework classes, just like you should add a prefix to the names of your own classes. You might choose to use the same three letters you use for your class prefixes, but lowercase to follow the usual convention for method names, then an underscore, before the rest of the method name."
https://developer.apple.com/library/archive/documentation/Cocoa/Conceptual/ProgrammingWithObjectiveC/CustomizingExistingClasses/CustomizingExistingClasses.html

I think we should consider moving this to the objc module ([rename_check.py](https://github.com/llvm/llvm-project/blob/master/clang-tools-extra/clang-tidy/rename_check.py) might be used if we decide that the move is justified).


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

https://reviews.llvm.org/D65917