Skip to content

fix memory leaks #178

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

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

fix memory leaks #178

wants to merge 4 commits into from

Conversation

kyoji2
Copy link

@kyoji2 kyoji2 commented Oct 4, 2019

No description provided.

Copy link
Owner

@yannickl yannickl left a comment

Choose a reason for hiding this comment

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

Oh great! Why I have never seen that before?!

Can you address the few comments before I merge them?

Comment on lines +204 to 205
guard let self = self else { return }
guard !self.session.isRunning else { return }
Copy link
Owner

Choose a reason for hiding this comment

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

I prefer one line guard. Is it possible to inline the first statement?

guard let self = self, !self.session.isRunning else { return }

Comment on lines +218 to 219
guard let self = self else { return }
guard self.session.isRunning else { return }
Copy link
Owner

Choose a reason for hiding this comment

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

The same here.

guard let self = self, self.session.isRunning else { return }

@dvertovsek
Copy link

dvertovsek commented Apr 22, 2020

Hey @yannickl I would be very happy if these changes made it to master soon.

Any news on this ?

@kyoji2

@hansemannn
Copy link

@yannickl May I ask you to merge the changes and apply the two changes afterwards? The memory issue is quite severe. Thank you!

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