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

Implemented a basic ref-counting system to terminate glfw #128

Merged
merged 1 commit into from
Oct 19, 2019
Merged

Implemented a basic ref-counting system to terminate glfw #128

merged 1 commit into from
Oct 19, 2019

Conversation

Isho312
Copy link
Contributor

@Isho312 Isho312 commented Sep 15, 2019

this was a known issue, and was fixed in #125

@LovelySanta LovelySanta added Impact:Feature Adds new content Impact:Bug Unintended behaviour labels Sep 15, 2019
@CleverSource CleverSource mentioned this pull request Sep 15, 2019
5 tasks
@LovelySanta LovelySanta added this to Bugfixes (awaiting merge) in Community additions Sep 15, 2019
@reductor
Copy link

This seems like a the global init should be split from Window

@LovelySanta
Copy link
Collaborator

This seems like a the global init should be split from Window

This is how cherno did initialize it for now. I agree, we should have an abstract GLFW window base class inbetween Window and WindowWindow, but that is up to cherno to decide. For now this is a good implementation, as it was like this previously as well.

@WhoseTheNerd
Copy link
Contributor

I would recommend System class that initializes all required dependencies(dependencies that need to be manually initialized)

@LovelySanta
Copy link
Collaborator

LovelySanta commented Sep 19, 2019

I would recommend System class that initializes all required dependencies(dependencies that need to be manually initialized)

I agree, but that is more of a feature. Imo, this is still a valid "get things up and running" PR. When some sort of System is in place, this will have to be removed. I rather have glfw terminating correctly than having memory leaks we know we can prevent in the first place.

@reductor
Copy link

imho, I see this as technical debt over being a 'feature', this commit just compounding the technical debt.

This is an end-of-application clean-up so not really a memory leak just a false-positive with no side-effects that can show up in leak detection

@LovelySanta
Copy link
Collaborator

This is an end-of-application clean-up so not really a memory leak just a false-positive with no side-effects that can show up in leak detection

I know, I just want to work on closing that memory leak thread, the less results, the faster we can solve it.

@LovelySanta LovelySanta merged commit c041819 into TheCherno:master Oct 19, 2019
Community additions automation moved this from Bugfixes (awaiting merge) to Merged PR's Oct 19, 2019
@Isho312 Isho312 deleted the memory-leaks-patch_glfw-termination branch April 12, 2020 03:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Impact:Bug Unintended behaviour Impact:Feature Adds new content
Projects
Community additions
  
Merged PR's
Development

Successfully merging this pull request may close these issues.

None yet

4 participants