Such method declarations don't provide any benefit, as even without the declaration the compiler would complain about calling the method as it doesn't exist.
|440 ms||Clang Tools.clang-tidy/checkers::objc-method-unavailable-not-override.m|
Script: -- : 'RUN: at line 1'; /usr/bin/python /mnt/disks/ssd0/agent/workspace/amd64_debian_testing_clang8/clang-tools-extra/test/../test/clang-tidy/check_clang_tidy.py /mnt/disks/ssd0/agent/workspace/amd64_debian_testing_clang8/clang-tools-extra/test/clang-tidy/checkers/objc-method-unavailable-not-override.m objc-method-unavailable-not-override -target x86_64-apple-macosx /mnt/disks/ssd0/agent/workspace/amd64_debian_testing_clang8/build/tools/clang/tools/extra/test/clang-tidy/checkers/Output/objc-method-unavailable-not-override.m.tmp
Fix the test case.
Should probably be in an anonymous namespace this as you don't intend it to be visible in another translation unit.
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
This is not a satisfactory check, it will ensure at least one method has been deleted but it wont ensure all methods have been deleted. Would probably be safer putting a unique comment on the line and having a check fix that checks for an empty line and the comment for each case.
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.
Thanks for the suggestion! Done.
return llvm::is_contained(Macros, Tok.getRawIdentifier().str())
Should probably return an llvm::Optional<FixItHint>
This check is surplus to requirements now that there is the isLanguageVersionSupported check
objcMethodDecl is implicitly an allOf matcher, so there is no need to specify allOf
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)?
This looks wrong to me -- it's a semicolon-delimited list, isn't it?
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)?
Not a point for this patch, but maybe this should be made into an actual matcher, though not complimented, or better yet the CXXMethodDecl isOverride matcher be extended for ObjCMethodDecl
After some discussion, have decided to remove the fix-it entirely and update the diagnostic message; removing the method altogether may not be the correct behavior, as previously deprecated methods that have since been removed may want to have an unavailable attribute attached with a message explaining what to use instead, even though they don't override a superclass method.
But in general still feel like non-overriding, non-messaged attributes are probably reasonable to flag.
I agree, and also that it's probably best left to a later patch. Not sure how to deal with the making one that works for both CXXMethodDecl and ObjCMethodDecl.
Removing the attribute is undesirable, as that would make the compiler allow calling the method, when it has explicitly been marked unavailable. In my experience the majority of the cases in Objective-C for using this attribute are to mark a superclass method unavailable (e.g. a designated initializer that shouldn't be called on the subclass, even just -init, favoring its own declared designated initializer instead), but it can also be used to mark a previously-deprecated-now-removed method with a message to point updaters what method to migrate to.
Which brings up an interesting point that if a message is provided, it could be that an API has finally finished allowing a deprecated method to be called and removed it entirely, and so if a message is provided it may be indicating that and telling what new API to use, rather than overriding the availability of a superclass method. But a method being marked unavailable without any message doesn't really provide much help on that versus the method just not existing. Perhaps if a message is provided this check shouldn't complain at all.
I've removed the fix-it entirely, and replaced it with a different warning message.
Obsolete, fix-it deleted.