[PATCH] D75569: [clang-tidy] New check for methods marked __attribute__((unavailable)) that do not override a method from a superclass.

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.


================
Comment at: clang-tools-extra/clang-tidy/objc/MethodUnavailableNotOverrideCheck.cpp:63
+  }
+  return FixItHint::CreateRemoval(MD->getSourceRange());
+}
----------------
I'm not an ObjC expert, so I apologize if this is a dumb question, but why is the fix-it removing the entire method as opposed to just the attribute?

Also, are you sure that the source range for the method declaration is sufficient to remove without breaking code? e.g., what about the definition of the method? Or can there be any trailing bits after the method that are not accounted for in the source range (such as trailing attributes, if those are possible in ObjC)?


================
Comment at: clang-tools-extra/docs/clang-tidy/checks/objc-method-unavailable-not-override.rst:29
+
+  Comma-separated list of names of macros that can define the unavailable
+  attribute for which fixes should be suggested. The default is an empty list.
----------------
This looks wrong to me -- it's a semicolon-delimited list, isn't it?


================
Comment at: clang-tools-extra/test/clang-tidy/checkers/objc-method-unavailable-not-override.m:34
+// Verify check when using a macro that expands to the unavailable attribute.
+- (void)methodC NS_UNAVAILABLE;
+// CHECK-MESSAGES: :[[@LINE-1]]:1: warning: method 'methodC' is marked unavailable but does not override a superclass method [objc-method-unavailable-not-override]
----------------
mwyman wrote:
> njames93 wrote:
> > Generally we are cautious about modifying MACRO usages in clang_tidy as we don't know if its definition can change based on configuration, perhaps its safer to just warn instead of providing a fix it
> Sounds reasonable; I made this the default behavior.
> 
> However for Objective-C, it's quite common for developers to use a macro from the Apple SDKs like NS_UNAVAILABLE that are unconditional in any situations I know of. I added a config option to allow whitelisting macros that should have fix-its provided.
If the common practice is to use macros like NS_UNAVAILABLE, should that be on the default list of options (along with any other common-use macro names that do the same thing)?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D75569/new/

https://reviews.llvm.org/D75569