stephanemoore added a comment.
Looks in good shape 👌 A couple nits and polish ideas.
================
Comment at: clang-tools-extra/clang-tidy/objc/DeallocInCategoryCheck.cpp:34
+
+void DeallocInCategoryCheck::check(const MatchFinder::MatchResult &Result) {
+ const auto *MatchedDecl = Result.Nodes.getNodeAs<ObjCMethodDecl>("dealloc");
----------------
What do you think of making a fixit that deletes the `-dealloc` implementation?
================
Comment at: clang-tools-extra/clang-tidy/objc/DeallocInCategoryCheck.cpp:36
+ const auto *MatchedDecl = Result.Nodes.getNodeAs<ObjCMethodDecl>("dealloc");
+ if (MatchedDecl) {
+ diag(MatchedDecl->getLocation(),
----------------
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:
https://github.com/llvm/llvm-project/blob/master/clang-tools-extra/clang-tidy/objc/MissingHashCheck.cpp#L54
https://github.com/llvm/llvm-project/blob/master/clang-tools-extra/clang-tidy/objc/ForbiddenSubclassingCheck.cpp#L73
================
Comment at: clang-tools-extra/clang-tidy/objc/DeallocInCategoryCheck.cpp:37-38
+ if (MatchedDecl) {
+ diag(MatchedDecl->getLocation(),
+ "method -dealloc should not be implemented in a category");
+ }
----------------
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)
================
Comment at:
clang-tools-extra/docs/clang-tidy/checks/objc-dealloc-in-category.rst:7
+Finds implementations of ``-dealloc`` in Objective-C categories. The category
+implementation will override any dealloc in the class implementation,
+potentially causing issues.
----------------
``-dealloc``
================
Comment at:
clang-tools-extra/docs/clang-tidy/checks/objc-dealloc-in-category.rst:10-11
+
+Classes implement ``-dealloc`` to perform important actions just before an
+object is deallocated, but if a category on the class implements ``-dealloc``
+it will override the class's implementation and those important actions may
----------------
Replace "just before an object is deallocated" with "to deallocate an object".
"Deallocates the memory occupied by the receiver."
https://developer.apple.com/documentation/objectivec/nsobject/1571947-dealloc?language=objc
================
Comment at:
clang-tools-extra/docs/clang-tidy/checks/objc-dealloc-in-category.rst:11
+Classes implement ``-dealloc`` to perform important actions just before an
+object is deallocated, but if a category on the class implements ``-dealloc``
+it will override the class's implementation and those important actions may
----------------
Replace ", but if" with ". If"?
================
Comment at:
clang-tools-extra/docs/clang-tidy/checks/objc-dealloc-in-category.rst:11
+Classes implement ``-dealloc`` to perform important actions just before an
+object is deallocated, but if a category on the class implements ``-dealloc``
+it will override the class's implementation and those important actions may
----------------
stephanemoore wrote:
> Replace ", but if" with ". If"?
I //think// we should add a comma after ``-dealloc``?
================
Comment at:
clang-tools-extra/docs/clang-tidy/checks/objc-dealloc-in-category.rst:12-13
+object is deallocated, but if a category on the class implements ``-dealloc``
+it will override the class's implementation and those important actions may
+not be handled by the overriding ``-dealloc``.
----------------
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"?
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D72876/new/
https://reviews.llvm.org/D72876
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
