Convert resolveInstanceMethod in TyphoonAssembly to forwardInvocation/NSInvocation

[jasperblues]

This change will:

• Make the code easier to read and understand
• Support an arbitrary number of runtime args in a single method. (Current we dispatch to 11 separate function pointers depending on the number of args).
• Support primitive runtime args.

[jasperblues]

Perhaps we should use: https://github.com/steipete/Aspects

[jasperblues]

Also this: http://codeshaker.blogspot.co.at/2012/01/aop-delivered.html

[jasperblues]

Another interesting link: https://github.com/OliverLetterer/SPLMessageLogger

[alexgarbarev]

I dislike the last solution, since it platform dependent. Previous two links uses same approach, and they used forwarding NSInvocation (but seems that they used private API?), as we discussed before. I prefer writing own implementation to not depend on another library, lightweight and code simplicity/readability.

[jasperblues]

I think the last one uses custom jump tables (platform dependent assembly) and I only included it because it looked interesting and complicated.

The others were serious suggestions. As far as I know the library doesn't use any private APIs though it might have platform issues.

Anyway, I agree it's better to write a simple NSInvocation/forwarding solution.

[alexgarbarev]

I'll write full answer when come back to Omsk.

четверг, 8 мая 2014 г. пользователь Jasper Blues написал:

I think the last one uses custom jump tables (platform dependent assembly)
and I only included it because it looked interesting and complicated.

The others were serious suggestions. As far as I know the library doesn't
use any private APIs though it might have platform issues.

Anyway, I agree it's better to write a simple NSInvocation/forwarding
solution.

—
Reply to this email directly or view it on GitHubhttps://github.com//issues/214#issuecomment-42470336
.

Sincerely, Aleksey

[jasperblues]

I recall now that I started on this last y ear, during early plans to capture run-time args. Not that I anticipated the current problem with va_args - I didn't realize that was an option. . . because I thought it would be neater than having a table of IMPs to jump to.

I started using a proxy-based approach, but ran into problems:

http://stackoverflow.com/questions/18295700/internal-calls-on-my-proxied-class-dont-get-routed-through-proxy

[alexgarbarev]

Unit tests are passed, but I think we should test changes on real projects on 64bit devices, because it was a key change ( affects to whole typhoon )

[jasperblues]

Will convert the tests so that they can be run on device in the coming days.

Meanwhile, testing on current project.

[jasperblues]

Runtime args fail if one of the args is nill. This (presumably) is irrespective of platform (32 bit or 64bit).

[alexgarbarev]

Strange. I have test for nil arguments (named test_runtime_knight_with_nil) but it was success. Can you check/add failed test?

[alexgarbarev]

Ha. Got it. It crashes when argument is method parameter with
test failure: -[RuntimePropertiesInjectionsTests test_runtime_knight_with_method_arg_nil] failed: *** -[__NSArrayM replaceObjectAtIndex:withObject:]: object cannot be nil

right? Have you same crash?

[jasperblues]

Yes, same crash.

[alexgarbarev]

Good catch! This crash not related to latest changes - it can be reproduced on released code.
Anyway, now it fixed. You should check it again.

[jasperblues]

👍 A real project always helps. Though once we have the CI builds back in shape, I'll start increasing test coverage again.

[jasperblues]

Works fine!

[jasperblues]

bumping CocoaPods minor version.