Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Address static analyzer warning in OCPartialMockObject (Issue #543) #544

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

LowAmmo
Copy link

@LowAmmo LowAmmo commented Nov 12, 2024

OCPartialMockObject#setupForwarderForSelector

  • Modified to just return if there is no existing implementation for the method

* Issue erikdoe#543 about potential nil passed to class_addMethod()

OCPartialMockObject#setupForwarderForSelector
* Modified to just return if there is no existing implementation for the method
@LowAmmo
Copy link
Author

LowAmmo commented Nov 20, 2024

@erikdoe - Any chance this could be merged so it's in any future OCMock releases?

-Thanks!

@erikdoe
Copy link
Owner

erikdoe commented Nov 23, 2024

To be honest, it's a long time since I wrote that code, but returning early from the method if originalIMP is null – as suggested in the PR – might be a bit too simple. I mean, there is the code to create types when originalMethod is null, and the forwarder is installed in such cases.

A less risky/invasive change would be to skip only the call to class_addMethod when originalIMP is null. This could have a performance impact, though, because this would skip setting any entry for aliasSelector` which means the if statement at the top of the method wouldn't be true on later invocations.

I think my assumption was that it was okay to set NULL as an implementation and that would trigger a forwarding mechanism. Looking at the header file for class_addMethod, that assumption is clearly wrong, at least today, and the third parameter must not be null. But that also means that it probably never is null. So, skipping the class_addMethod only to make the analyzer happy seems a sensible idea. (Famous last words...)

@LowAmmo
Copy link
Author

LowAmmo commented Dec 3, 2024

@erikdoe - You've got a much better understanding of the innerworkings of all of this than I do, so I will gladly defer to your gut instincts! 😄

I do suspect you're right, that there is enough other "safety" checks that just skipping the call to "class_addMethod" is likely okay in this erroneous instance of a consumer trying to mock a method that doesn't exist (or can't be found) via ObjC. Especially since it doesn't seem to be causing problems, today - other than the warning message I'm trying to clean up. 😄

…if original implementation couldn't be found
@LowAmmo
Copy link
Author

LowAmmo commented Dec 12, 2024

@erikdoe - Just hoping to get this approved & pulled in & released to cocoapods sometime soon?

-Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants