Page MenuHomePhabricator

[Sema][ObjC] Disable -Wunused-parameter for ObjC methods
ClosedPublic

Authored by ahatanak on Apr 25 2019, 1:41 PM.

Diff Detail

Repository
rL LLVM

Event Timeline

ahatanak created this revision.Apr 25 2019, 1:41 PM

but don't want to annotate each unused ObjC method parameters with __attribute__((unused)) or insert (void)unused_param into the body of the method to silence the warning since, unlike C/C++ functions, it's not possible to comment out the unused parameters of ObjC methods.

Thats also true of function parameters in C (and Objective-C), so I don't think that argument is very convincing. I think it is true that this is a much less useful diagnostic for methods though, because they're more likely to be overriding something that requires a certain selector. Maybe we should just suppress this warning in general for methods (and even virtual functions in C++)?

Yeah, I tend to think we should just suppress this unconditionally in Objective-C.

Yeah, I tend to think we should just suppress this unconditionally in Objective-C.

IMO this warning still makes sense for normal functions, or methods that are only declared in an @implementation. Adding a fix-it to cast to void in the function/method body would probably go a long way too.

Sorry, yes, I meant in Objective-C methods, of course, not unconditionally even in C-like code whenever Objective-C is enabled.

Yeah, I tend to think we should just suppress this unconditionally in Objective-C.

IMO this warning still makes sense for normal functions, or methods that are only declared in an @implementation. Adding a fix-it to cast to void in the function/method body would probably go a long way too.

Should we use the new warning to warn just about unused ObjC method parameters and have -Wunused-parameter warn about everything else? If we make -Wunused-parameter unconditionally ignore ObjC methods, a user might complain about it later if -Wunused-parameter didn't diagnose an unused parameter when it should have.

Yeah, I tend to think we should just suppress this unconditionally in Objective-C.

IMO this warning still makes sense for normal functions, or methods that are only declared in an @implementation. Adding a fix-it to cast to void in the function/method body would probably go a long way too.

Should we use the new warning to warn just about unused ObjC method parameters and have -Wunused-parameter warn about everything else? If we make -Wunused-parameter unconditionally ignore ObjC methods, a user might complain about it later if -Wunused-parameter didn't diagnose an unused parameter when it should have.

Yeah, that seems fine to me, leave the method diagnostic off-by-default, since I doubt many would actually want it. Do you think its worth emitting -Wunused-parameter in pure-@implementation methods?

Yes, I guess we can make -Wunused-parameter cover unused parameters of pure @implementation methods too since users can probably just remove them to silence the warning.

I do not think the ObjC version of this diagnostic is useful as it's been explained here, and I would strongly recommend just not including such a warning for the time being.

I would also recommend that you go fix the warning to never fire on virtual C++ methods.

I do not think the ObjC version of this diagnostic is useful as it's been explained here, and I would strongly recommend just not including such a warning for the time being.

Why? It seems to me like the vast majority of methods only declared in an @implementation are used as local helpers (even if people probably should be using functions for this), where this warning would be useful. Maybe I'm missing something? Still trying to learn more about Objective-C.

I do not think the ObjC version of this diagnostic is useful as it's been explained here, and I would strongly recommend just not including such a warning for the time being.

Why? It seems to me like the vast majority of methods only declared in an @implementation are used as local helpers (even if people probably should be using functions for this), where this warning would be useful. Maybe I'm missing something? Still trying to learn more about Objective-C.

What rule are you suggesting for whether a method is "only declared in an @implementation"? Where exactly are you proposing to look for other declarations?

Objective-C does not have a formalized notion of an @implementation-private method, or really even an unambiguous convention for them (underscoring is *library*-private by convention, not class-private). The Objective-C ecosystem includes a lot of informal protocols and conventions based around discovery via naming, and Objective-C programmers do (admittedly rarely) just override methods that they know are there but which they can't see. These things make me very worried about trying to port assumptions from other languages.

Anyway, put that aside for a minute. As I understand it, your current proposal is to make -Wunused-parameter not fire on the majority of Objective-C methods and then add a new -Wunused-objc-parameter warning that's not really formally related to -Wunused-parameter. I think the right way to think about that is that the new warning is an independent feature which can happen at its own pace. You will need to do some investigation in order to implement that feature, so in the short term, you should just not warn about unused parameters in Objective-C methods, and then you can do the investigation for generalizing the warning as time permits.

I do not think the ObjC version of this diagnostic is useful as it's been explained here, and I would strongly recommend just not including such a warning for the time being.

Why? It seems to me like the vast majority of methods only declared in an @implementation are used as local helpers (even if people probably should be using functions for this), where this warning would be useful. Maybe I'm missing something? Still trying to learn more about Objective-C.

What rule are you suggesting for whether a method is "only declared in an @implementation"? Where exactly are you proposing to look for other declarations?

Objective-C does not have a formalized notion of an @implementation-private method, or really even an unambiguous convention for them (underscoring is *library*-private by convention, not class-private). The Objective-C ecosystem includes a lot of informal protocols and conventions based around discovery via naming, and Objective-C programmers do (admittedly rarely) just override methods that they know are there but which they can't see. These things make me very worried about trying to port assumptions from other languages.

Concretely, I was just thinking of using something like ObjCMethodDecl::isOverriding, but if that isn't a good enough heuristic for finding local methods then I guess we should just forgo methods entirely.

ahatanak updated this revision to Diff 197469.Apr 30 2019, 3:53 PM
ahatanak retitled this revision from [Sema][ObjC] Add a flavor of -Wunused-parameter that doesn't diagnose unused parameters of ObjC methods to [Sema][ObjC] Disable -Wunused-parameter for ObjC methods.
ahatanak edited the summary of this revision. (Show Details)

Disable the warning for ObjC methods.

erik.pilkington accepted this revision.Apr 30 2019, 3:58 PM

LGTM, thanks!

This revision is now accepted and ready to land.Apr 30 2019, 3:58 PM

I would also recommend that you go fix the warning to never fire on virtual C++ methods.

I tried building clang/llvm with -Wunused-parameter turned on and there are lots of places where parameters are unused other than virtual functions, in particular template functions and non-template functions that are called by other template functions. So disabling the warning on C++ virtual functions is probably not enough in order to make this warning more useful.

This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptMay 3 2019, 12:20 AM