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

Added supports for ARC #12

Merged
merged 1 commit into from
Mar 28, 2012
Merged

Added supports for ARC #12

merged 1 commit into from
Mar 28, 2012

Conversation

kajinka13
Copy link

Hello your work seems very good,
ARC does not support, so I made ​​this small change if you're interested.

@ylechelle
Copy link
Owner

Perfectly relevant. Thanks for the patch.

@ylechelle ylechelle closed this Mar 28, 2012
@ylechelle ylechelle reopened this Mar 28, 2012
ylechelle added a commit that referenced this pull request Mar 28, 2012
@ylechelle ylechelle merged commit 98b77d2 into ylechelle:master Mar 28, 2012
@steipete
Copy link

Sorry, but why is this needed, like really? Everyone can just use -fno-objc-arc or -f-objc-arc and it works beautifully. Using macros to have both ARC and Non-ARC completely defeats the purpose of ARC, thus being less work for the developer.

@mattjgalloway
Copy link
Contributor

Agreed. Not only that but this commit pollutes any code that imports OpenUDID.h with some #defines that have not even been attempted to be namespaced in any way (e.g. prefix with OUD_ or something).

@3lvis
Copy link

3lvis commented Mar 30, 2012

Where are the down votes for pull requests?

@ylechelle
Copy link
Owner

I agree that devs can selectively compile for ARC or not, but it's more of a pain to integrate for them.
Not everyone has upgraded to a version of Xcode that supports ARC; and I'm sure lots will keep using non-ARC code for a while.

Alternative is to have one ARC version and one non-ARC version, which is a pain to maintain.

I'm in favor to keep this simple define trick.
However, Matt, you're right. The defines have nothing to do in the header file!!!
Moving them right away into the .m file so that they no longer have an impact in other parts of the hosting project.

Makes sense?

@mattjgalloway
Copy link
Contributor

Moving the defines to the implementation file is definitely a good idea. Personally I agree with @steipete and just choose one and then stick an error at the top that is caught if you're building for ARC and the code is chosen to not support ARC or vice-versa.

Your argument about not upgrading to a version of Xcode that supports ARC is invalid IMO since you need to be using the latest Xcode to submit to the App Store anyway. Also, people should upgrade anyway :-).

@ylechelle
Copy link
Owner

ok ;-)

So let's be pragmatic. Let's keep these convenience #define for a while in the .m and clean up the code to be fully ARC-based in a few months? Say after WWDC 2012? How's that?

@steipete
Copy link

ARC is supported since Xcode 4.2. If you're using anything older, you're doing it wrong. Really. (and good call on the submission, @mattjgalloway) Just choose one (i'd go for ARC anytime) and add a ifdef that error's if it's being compiled w/o ARC. People usually don't add OpenUDID multiple times each day, so it's not really annoying. And if you're using ARC, there is no issue at all. More an "issue" for legacy apps. I fear that if you change that later, it probably does more harm that going with the right solution initially.

@mattjgalloway
Copy link
Contributor

Couldn't agree more. Also, if you make the error provide instructions for what to do then the user will be able to fix it. e.g.:

#if ! __has_feature(objc_arc)
#error This file requires ARC to be enabled. Either enable ARC for the entire project or use -fobjc-arc flag.
#endif

@mattjgalloway
Copy link
Contributor

I've put in a pull request with how I would do this - #20

For what it's worth, your current code wouldn't compile under ARC anyway since a bridging cast was missing.

@ylechelle
Copy link
Owner

Ok, your arguments won me over. Merging now ;-)

@ylechelle
Copy link
Owner

We now have the issue of trying to create a code base that works with legacy non-arc libraries that are NOT pre-compiled, and therefore require younger developers (beginners) to meddle with the flags.
The #define approach was definitely more flexible... Could everyone in this thread live with a dynamic ARC code if the #define are in the implementation file???

@steipete
Copy link

No. I'd be hard on that one. It's REALLY not hard to set a flag on a file. If people don't get that, they shouldn't write apps at all.

@3lvis
Copy link

3lvis commented Apr 11, 2012

Is not about if it's easy or difficult. Is about best practices and how things have to be done.

@ylechelle
Copy link
Owner

I'm not suggesting it is hard for any of us in this thread. I'm talking about lib developers who will have to keep integrating the latest commit and go backwards for their users (in the case of libs distributed as source code - and there are many).

You still haven't answered my question re: the pragmatic #define approach? Can you live with it or is is "difficult" for you? ;-)

My feeling is that we need to go back to the pragmatic approach for a few more months...

@jcgsxr
Copy link

jcgsxr commented Apr 11, 2012

On a related note, we manage and distribute an SDK as part of integrating our ad products into applications. A number of developers in our ad network have users who are running iOS versions 4.2 and below, which causes crash issues for them.

Whether or not using ARC is considered a best practice, this becomes a blocking issue for us as we can't just cut out a portion of our user base. I can only assume that this would also be a blocking issue for others as well who are potentially considering OpenUDID, since we must support all devices and OS versions at the end of the day.

I agree with Yann, in that the #define approach is more flexible. I think it's all about making a solution like OpenUDID more widely accepted, instead of drawing some hard line by enforcing a specific coding practice.

@steipete
Copy link

How's your crash problem related to ARC? ARC works down all the way till iOS4, Xcode is clever enough to add the small helper library libarclite.a to support older devices.

@mattjgalloway
Copy link
Contributor

@ylechelle - how do they have to go back and keep integrating the latest commit? People who are not using ARC in their main project just need to add -fobjc-arc flag to the compile flags for the files contained within OpenUDID and then that is it. (And need to link with -fobjc-arc set as well for the 4.x backwards compatibility to be compiled in.). I don't see why that's hard?

@jcgsxr - if you're having crashing bugs, then fix them. The fix will not be "disable ARC" - there must be something wrong somewhere.

#define is not a pragmatic approach, IMO. It's just making it easy for developers to miss out on slowly migrating to ARC. Remember, they don't need to convert their whole project to ARC - they can just compile OpenUDID with ARC enabled. That to me is a nice little introduction that everyone should be doing. And if they can't do it, then as @steipete suggests - they shouldn't be writing apps.

@jcgsxr
Copy link

jcgsxr commented Apr 11, 2012

I may have misspoke. To clarify, compiling and attempting to run this code on devices running iOS 3.x won't work.

@ylechelle
Copy link
Owner

@mattjgalloway - direct users of OpenUDID might find this acceptable. Might. But users of lib A or B that embeds OpenUDID and does NOT require ARC to compile would be forced to add this compile flag JUST for OpenUDID and developers of lib A or B (big ones actually, not naming names) are not ready to enforce ARC compiling just yet... agreed it's the way forward, but even Apple does not force ARC by default (many existing samples, etc...).

@mattjgalloway @steipete you guys should run a separate branch that is ARC by default. How's that?

@mattjgalloway
Copy link
Contributor

@ylechelle But still, you just need to enforce it on the OpenUDID compilation units - which is only 1 file. That's not a big deal and everyone should be comfortable doing this, IMO.

If only there was a #pragma for turning on ARC for a file, then we wouldn't have to be having this debate...

@steipete
Copy link

They're not. In that case the library would add the compatibility libraries by default, and the user wouldn't have to care about that at all. But I agree that this can be troublesome, because there's a problem when those are not used as submodules but precompiled binary blobs; in that case it could happen that multiple libraries have arc compatibility libs and one would need to use some command line tools to make it work. (thank god this problem goes away as we drop iOS4)

Apple doesn't force ARC, but I bet everything that you'll never see sample code using ARC macros. If you're worried about the above, just remove ARC support. Better than mixing both worlds.

@ylechelle
Copy link
Owner

Ok, now we're talking ;-) The main concern right now is to help MOST developers adopt OpenUDID. So I'm in favor of going non-ARC or macros. @mattjgalloway @NSElvis please weigh in!

@mattjgalloway
Copy link
Contributor

Absolutely. Pick ARC or non-ARC and stick with it. I believe that was the original stance in this conversation :-). If you want 100% support for non-ARC, then choose non-ARC.

@ylechelle
Copy link
Owner

Ok. Sorry @kajinka13
We're going back to non-ARC.
Makes sense after all.

@mattjgalloway
Copy link
Contributor

I suggest having an ARC branch as well so that at least it'll be easy to switch over / people can take their pick.

@ylechelle
Copy link
Owner

Time permitting yes. But I think in a few months (post-WWDC, post iOS 6?), we'll be ready to move to ARC fully as well...

@myell0w
Copy link

myell0w commented Apr 11, 2012

100 % on @steipete's side here: pick either ARC, or non-ARC, no macros, no separate branch for ARC. And for libraries created now, ARC should be used as Apple is moving towards ARC everywhere.

@ylechelle
Copy link
Owner

We're back to non-ARC folks.

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.

None yet

7 participants