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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
