mwyman added inline comments.
================
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");
----------------
stephanemoore wrote:
> What do you think of making a fixit that deletes the `-dealloc`
> implementation?
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.
================
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");
+ }
----------------
stephanemoore wrote:
> 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)
I think it's a great idea! 🙌
Done!
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
