[PATCH] D72876: Create a clang-tidy check to warn when -dealloc is implemented inside an ObjC class category.

[Michael Wyman via Phabricator via cfe-commits]

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