Such implementations may override the class's own implementation, and even be a danger in case someone later comes and adds one to the class itself. Most times this has been encountered have been a mistake.
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
- Build Status
Buildable 45876 Build 48001: arc lint + arc unit
Event Timeline
clang-tools-extra/docs/clang-tidy/checks/objc-dealloc-in-categories.rst | ||
---|---|---|
10 ↗ | (On Diff #238611) | Need an end quote on dealloc |
clang-tools-extra/clang-tidy/objc/DeallocInCategoriesCheck.cpp | ||
---|---|---|
21 ↗ | (On Diff #238621) | Add isInstanceMethod() within the objcMethodDecl? |
clang-tools-extra/clang-tidy/objc/DeallocInCategoriesCheck.h | ||
24 ↗ | (On Diff #238621) | What do you think of the name DeallocInCategoryCheck? |
clang-tools-extra/test/clang-tidy/checkers/objc-dealloc-in-categories.m | ||
23 ↗ | (On Diff #238621) | Please add a class method +dealloc and make sure that the check does not trigger on that. |
35 ↗ | (On Diff #238621) | Perhaps add an @implementation for Baz (not a category implementation) that contains -dealloc to ensure that a declaration in a category with a definition in the implementation does not trigger? |
clang-tools-extra/clang-tidy/objc/DeallocInCategoriesCheck.cpp | ||
---|---|---|
19 ↗ | (On Diff #238621) | Early return if the language is not an Objective-C variant: // This check should only be applied to Objective-C sources. if (!getLangOpts().ObjC) return; |
21 ↗ | (On Diff #238621) | Technically, isn't -dealloc specific to certain classes, e.g., [NSObject](https://developer.apple.com/documentation/objectivec/nsobject/1571947-dealloc?language=objc)) and NSProxy? 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. |
Updated the diff based on review feedback.
clang-tools-extra/clang-tidy/objc/DeallocInCategoriesCheck.cpp | ||
---|---|---|
21 ↗ | (On Diff #238621) | 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. |
clang-tools-extra/clang-tidy/objc/DeallocInCategoriesCheck.h | ||
24 ↗ | (On Diff #238621) | Much better. |
Looks in good shape 👌 A couple nits and polish ideas.
clang-tools-extra/clang-tidy/objc/DeallocInCategoryCheck.cpp | ||
---|---|---|
35 | What do you think of making a fixit that deletes the -dealloc implementation? | |
37 | I can't speak authoritatively regarding project convention but I believe that it's rare to condition on a nonnull match when matching on a single AST node (in other words, check would not have been called if MatchedDecl were null). I believe that convention is either to omit the expected condition or to assert the expected condition. Examples: | |
38–39 | What do you think of binding the category implementation declaration and including that in the diagnostic message? Finder->addMatcher(objcMethodDecl(isInstanceMethod(), hasName("dealloc"), hasDeclContext(objcCategoryImplDecl().bind("impl"))) .bind("dealloc"), this); // ... const auto *CID = Result.Nodes.getNodeAs<ObjCCategoryImplDecl>("impl"); diag(MatchedDecl->getLocation(), "category %0 should not implement -dealloc") << CID; (admittedly, I don't recall off the top of my head how ObjCCategoryImplDecl renders in diagnostic messages so some additional tuning could be warranted) | |
clang-tools-extra/docs/clang-tidy/checks/objc-dealloc-in-category.rst | ||
8 | `-dealloc` | |
11–12 | Replace "just before an object is deallocated" with "to deallocate an object". "Deallocates the memory occupied by the receiver." | |
12 | Replace ", but if" with ". If"? | |
12 | I think we should add a comma after `-dealloc`? | |
13–14 | Isn't it a bit more nefarious than that? -dealloc in the main implementation becomes inaccessible (though the superclass can still be invoked). Perhaps it would suffice to say that "unexpected deallocation behavior may occur"? |
Implemented review feedback.
Updated diagnostic message to mention the category name.
clang-tools-extra/clang-tidy/objc/DeallocInCategoryCheck.cpp | ||
---|---|---|
35 | I would assume any non-empty -dealloc has some code the developer would want to migrate elsewhere rather than just delete, so I would prefer not to offer to outright delete it. Might be good for empty -dealloc methods or ones that just call super (pre-ARC, bah humbug!)—but I'd prefer to take that up in a follow-up change. | |
38–39 | I think it's a great idea! 🙌 Done! |
One last correction and I think that you're all set!
clang-tools-extra/docs/clang-tidy/checks/list.rst | ||
---|---|---|
284 | Please revert this line. |
LGTM, just one nit-pick.
clang-tools-extra/clang-tidy/objc/DeallocInCategoryCheck.h | ||
---|---|---|
25 | class DeallocInCategoryCheck final |
class DeallocInCategoryCheck final