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.

Sign up for GitHub

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

Jump to bottom

[3.2] [iOS] Leaks and deprecations fix #41504

Merged
merged 1 commit into from
Aug 25, 2020
Merged

[3.2] [iOS] Leaks and deprecations fix #41504

merged 1 commit into from
Aug 25, 2020

Conversation

naithar
Copy link
Contributor

@naithar naithar commented Aug 25, 2020 •
edited
Loading

Fixes some iOS warnings as well as leaks that was reported by Xcode Analyzer.

Some parts haven't been changed as they are addressed by other PRs, like #41173 or #41340

@naithar
Copy link
Contributor Author

naithar commented Aug 25, 2020 •
edited
Loading

Is there any reason to not use ARC (-fobjc-arc) and instead use MRR (-fno-objc-arc) for iOS/macOS?
This would remove the need for retain/release and possibly fix other leaks that's not reported by analyzer.
I've played around with ARC a little and it seems like it's not affecting how Godot is working.

Migrating some changes from #40434 as well as #40766 after #41340 gets merged should probably make cherry-picking iOS changes for 3.2 a bit easier.

@akien-mga akien-mga added this to the 3.2 milestone Aug 25, 2020
@akien-mga akien-mga requested a review from bruvzg August 25, 2020 12:31
bruvzg
bruvzg reviewed Aug 25, 2020
View reviewed changes
Copy link
Member

@bruvzg bruvzg left a comment

Choose a reason for hiding this comment

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

Everything else seems to be fine.

platform/iphone/view_controller.mm Outdated
p_args[p_argc] = NULL;
[str release];
Copy link
Member

@bruvzg bruvzg Aug 25, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

No, this should not be released here, it's intentionally leaked to preserve C-string.

Proper fix should do something like:

  1. In the iphone_main function: save original value of argc before calling add_path and add_cmdline.
  2. In add_path and add_cmdline: allocate memory and copy C-string (retain should be removed).
  3. In the iphone_main function, after Main::setup call: deallocate C-strings starting from argc saved in step one.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should early release result in crash or leak, which could be traced?

Copy link
Member

Choose a reason for hiding this comment

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

Actually there's no new NSString created and released here, objectForKey return existing key of [[NSBundle mainBundle] infoDictionary] which should be there while app is running, probably it's fine to just remove retain instead of adding release and no need to copy C-string.

Copy link
Contributor Author

@naithar naithar Aug 25, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Updated PR. Also tested it with custom godot_path and Instruments. Everything seems to be working correctly. Only InAppStore is reporting a leak at initializer, but I guess it could be fixed after #41340

platform/iphone/view_controller.mm Outdated
@@ -65,7 +66,8 @@ int add_cmdline(int p_argc, char **p_args) {
if (!str)
continue;
[str retain]; // @todo delete these at some point
p_args[p_argc++] = (char *)[str cString];
p_args[p_argc++] = (char *)[str cStringUsingEncoding:NSUTF8StringEncoding];
[str release];
Copy link
Member

Choose a reason for hiding this comment

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

Same as above.

@naithar
iOS: fix deprecations and leaks
f81c6c5 
Fixes some deprecations and leaks reported by Xcode Analyzer
@naithar
Copy link
Contributor Author

naithar commented Aug 25, 2020

@bruvzg what do you think about using ARC for iOS? Or Godot should stick to manual memory management?

@bruvzg
Copy link
Member

bruvzg commented Aug 25, 2020

what do you think about using ARC for iOS? Or Godot should stick to manual memory management?

The only thing using retain is camera code (on both iOS and macOS), for the rest of code it should be OK. I do not think there's any particular reason ARC is not used, it's just old code that was created before ARC was introduced in OS X Lion.

@naithar
Copy link
Contributor Author

naithar commented Aug 25, 2020

Well, mixing ARC and non-ARC code is not really a problem, so I think it could be addressed for both 4.0 and 3.2 in time.

@akien-mga akien-mga merged commit 6b5102c into godotengine:3.2 Aug 25, 2020
@akien-mga
Copy link
Member

Thanks!

@naithar naithar mentioned this pull request Feb 3, 2021
Closed
@naithar naithar deleted the fix/analyzer-3.2 branch February 9, 2021 08:33
@akien-mga akien-mga modified the milestones: 3.2, 3.3 Apr 20, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bruvzg bruvzg bruvzg approved these changes

Assignees
No one assigned
Labels
bug enhancement platform:ios topic:buildsystem topic:porting
Projects
None yet
Milestone
3.2
Development

Successfully merging this pull request may close these issues.
3 participants
@naithar @bruvzg @akien-mga