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

Make PLBlockIMP drop in compatible on iOS 4 and 5 #2

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

Conversation

MSch
Copy link

@MSch MSch commented Jun 26, 2012

As discussed in IRC.

[20:22] msch: now if only someone would do the work of porting MAKVONotificationCenter to use landonf's PLBlockIMP i'd be a happy man

[20:24] landonf: msch: MAKVONotificationCenter ? that should be pretty easy. Just add pl_ to the front of its blockimp function calls. Or do something like what we did in PLWeakCompatibility to co-opt the symbol names, I guess.

I haven't yet had the opportunity to test on an iOS < 4.3 device.

#ifdef __MAC_OS_X_VERSION_MIN_REQUIRED
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We might be picking up the AvailabilityMacros.h header via the objc headers, but it would be nice to be explicit about including the header for these defines. Also, could these #if statements be somehow combined to avoid duplicating the typedefs?

As an alternative, the code could simply skip the duplicate declarations of the imp_* APIs if the SDK's _VERSION_MAX_ALLOWED is greater than 4.3 / 10.7.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the typedefs are still necessary, because otherwise the signature of the implementation of imp_implementationWithBlock and friends is mismatched on either iOS6+ or iOS<6.

Agreed on the rest, I don't know much (anything) about the preprocessor so I went the easy and verbose route.

@MSch
Copy link
Author

MSch commented Jun 27, 2012

Ok, just tested on our 4.2.1 device, works flawless.

@MSch
Copy link
Author

MSch commented Jul 1, 2012

So, what's the plan, can this be merged? What do I have to provide in order for it to be merged? Thanks!

@landonf
Copy link
Member

landonf commented Jul 11, 2012

Sorry, I'm slow, but it's not off my radar :)

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