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

Fix Objective-C/C++ codes to supporting ARC #6889

Merged
merged 23 commits into from
May 17, 2022

Conversation

2bbb
Copy link
Contributor

@2bbb 2bbb commented Mar 11, 2022

this PR makes goal to fix to support all not ARC codes as ARC codes.
please see discussion on: #6848

@2bbb 2bbb changed the title [W.I.P.] Supporting ARC [W.I.P.] Fix Objective-C codes to supporting ARC Mar 11, 2022
@ofTheo
Copy link
Member

ofTheo commented Mar 11, 2022

Thanks @2bbb !
Going to ping @danoli3 to take a look too, but as far as I can tell this looks good!

Copy link
Member

@danoli3 danoli3 left a comment

Choose a reason for hiding this comment

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

Looking great @2bbb ! Great work

Maybe just add this to the prime delegate of each platform and not in each obj-c converted but no issues as it's just safeguard

#if !__has_feature(objc_arc)
 #   error need ARC
 #endif

I'll check it out tomorrow and test vs some projects

So far looking good, reviewed changes

@2bbb
Copy link
Contributor Author

2bbb commented Mar 13, 2022

@danoli3

thanks for the comments.

Maybe just add this to the prime delegate of each platform and not in each obj-c converted but no issues as it's just safeguard

OK.
I will remove from .m/.mm and add ARC check macro in ofConstants.h like:

#ifdef TARGET_OSX
	#ifndef __MACOSX_CORE__
		#define __MACOSX_CORE__
	#endif
	#include <unistd.h>
	#include "GL/glew.h"
	#include <ApplicationServices/ApplicationServices.h>

	#if defined(__LITTLE_ENDIAN__)
		#define TARGET_LITTLE_ENDIAN		// intel cpu
	#endif
    
    #ifdef __OBJC__ // added
        #if !__has_feature(objc_arc)
            #error "need ARC"
        #endif
    #endif
#endif

...

#ifdef TARGET_OF_IOS
	#import <OpenGLES/ES1/gl.h>
	#import <OpenGLES/ES1/glext.h>

	#import <OpenGLES/ES2/gl.h>
	#import <OpenGLES/ES2/glext.h>


	#define TARGET_LITTLE_ENDIAN		// arm cpu

    #ifdef __OBJC__ // added
        #if !__has_feature(objc_arc)
            #error "need ARC"
        #endif
    #endif
#endif

I will test too, but do you think is this fine?

Copy link
Member

@ofTheo ofTheo left a comment

Choose a reason for hiding this comment

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

Could we change:
error need ARC
to something a bit more descriptive?
error Please enable ARC (Automatic Reference Counting) at the project level

@2bbb
Copy link
Contributor Author

2bbb commented Mar 15, 2022

@ofTheo
I will change this message when same as solving @danoli3 point out.

@danoli3
Copy link
Member

danoli3 commented Mar 17, 2022

Looking good, did initial tests. will continue before fully approving

We are now locked to iOS 13 + with inclusion of C++17 filepath just a FYI! No issue with this though!

@danoli3
Copy link
Member

danoli3 commented Mar 17, 2022

Might be a good idea to generate a guide to upgrade a project that is not ARC into ARC oF future though, I had some custom AppDelegates and views that forced into the new ARC realms. Will help devs

@2bbb 2bbb requested a review from ofTheo March 17, 2022 06:38
@2bbb
Copy link
Contributor Author

2bbb commented Mar 17, 2022

I updated about @danoli3 and @ofTheo point out about ARC feature checking.

@2bbb
Copy link
Contributor Author

2bbb commented Mar 17, 2022

@danoli3

962432e

I added some comments (can search with TODO:) for some points about delegate/protocol.
Those need to change with some non-trivial complexity and are worried about compatibility, I want to update after this PR (about ARC).
If you are interested in, please check it.

@2bbb
Copy link
Contributor Author

2bbb commented Mar 19, 2022

I think what replacing many pairs of an explicitly defined instance variable, @property and @synthesize to @property including implicitly defined instance variable and removing @synthesize is better to Maintainance.
because currently, if we need to change type of variable then we need to change two points, but if there is only one @property then we need to change once.

I will do this with other PR after this PR is finished.

@ofTheo
Copy link
Member

ofTheo commented May 5, 2022

Going to leave @danoli3 to okay the final merge of this!
@danoli3 just make sure to do Squash and Merge above with #changelog #osx #ios #tvos ( this helps with generating the changelog for releases )

@danoli3
Copy link
Member

danoli3 commented May 6, 2022

Screen Shot 2022-05-06 at 3 16 15 pm

Looks good! For anyone with issues with old non-arc code that cannot be converted: You can add this to compile flag per src file

-fno-objc-arc

@danoli3
Copy link
Member

danoli3 commented May 11, 2022

Been testing looks good

@danoli3 danoli3 changed the title [W.I.P.] Fix Objective-C codes to supporting ARC Fix Objective-C codes to supporting ARC May 12, 2022
@danoli3 danoli3 changed the title Fix Objective-C codes to supporting ARC Fix bjective-C codes to supporting ARC May 12, 2022
@danoli3 danoli3 changed the title Fix bjective-C codes to supporting ARC Fix Objective-C/C++ codes to supporting ARC May 12, 2022
@danoli3
Copy link
Member

danoli3 commented May 12, 2022

Good to merge @ofTheo

@ofTheo ofTheo merged commit 625df26 into openframeworks:master May 17, 2022
@ofTheo
Copy link
Member

ofTheo commented May 17, 2022

Thanks @2bbb @danoli3

@dimitre
Copy link
Member

dimitre commented Jul 21, 2022

Hello @2bbb I've tried to port ofxSyphon to ARC with no success.
As I need it for several projects I should change XCode project to disable ARC and comment out a line in ofConstants.h
My suggestion is we can disable ARC enforcing now in ofConstants.h as everything else works great anyway.
cc: @ofTheo

@2bbb
Copy link
Contributor Author

2bbb commented Jul 26, 2022

Hi, @dimitre

I changed about those on 96f11b8
but this was wrong.

maybe correct way is what condition macro is wrote in all .m / .mm file, or make one header and include from those files.
at least it was a completely mistake to put them together in ofConstants.h...

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.

4 participants