mwyman added a comment.
Updated the diff based on review feedback.
================
Comment at: clang-tools-extra/clang-tidy/objc/DeallocInCategoriesCheck.cpp:21
+ Finder->addMatcher(
+ objcMethodDecl(hasName("dealloc"),
hasDeclContext(objcCategoryImplDecl()))
+ .bind("dealloc"),
----------------
stephanemoore wrote:
> stephanemoore wrote:
> > Add `isInstanceMethod()` within the `objcMethodDecl`?
> Technically, isn't `-dealloc` specific to certain classes, e.g.,
> [`NSObject`](https://developer.apple.com/documentation/objectivec/nsobject/1571947-dealloc?language=objc))
> and
> [NSProxy](https://developer.apple.com/documentation/foundation/nsproxy/1589830-dealloc?language=objc)?
> If that is true, we should technically check that the category is on a class
> that is or is derived from a relevant class like `NSObject` or `NSProxy`.
Technically true, but given the prevalence of extant ObjC documentation talking
about the -dealloc method in terms of deallocation, it seems highly unlikely
any non-NSObject/NSProxy-rooted class hierarchies are going to use -dealloc in
any other context, and want to allow implementations in a category.
================
Comment at: clang-tools-extra/clang-tidy/objc/DeallocInCategoriesCheck.h:24
+///
http://clang.llvm.org/extra/clang-tidy/checks/objc-dealloc-in-categories.html
+class DeallocInCategoriesCheck : public ClangTidyCheck {
+public:
----------------
stephanemoore wrote:
> What do you think of the name `DeallocInCategoryCheck`?
Much better.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D72876/new/
https://reviews.llvm.org/D72876
_______________________________________________
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
