Skip to content
This repository has been archived by the owner on Jan 6, 2022. It is now read-only.

Resource leak patch #526

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

AlonzoTG
Copy link

Having learned a great deal about everything, especially the codebase, where the booby-traps are hidden, GIT, and the pull request process, this is an experimental pull request that, while maybe too small, addresses all concerns previously mentioned.

Addresses a potential resource leak to make sure containing class passes
destroy command to external resource.

Addresses a potential resource leak to make sure containing class passes
destroy command to external resource.
@malware-dev
Copy link

Adding IDisposable helps very little if Dispose is actually not being called 😄

@Jimmacle
Copy link

Jimmacle commented May 13, 2016

He's right, you'll need to go further into the code and make sure all those instances actually get disposed if you want your changes to have an effect. Remember, your tools are just giving suggestions - you can't let *them program for you, you need to combine it with your understanding of programming as well.

@Rynchodon
Copy link

Dispose is called by the destructor of FastResourceLock; there is no leak.

As is the case with FastResourceLock, the destructor is responsible for releasing unmanaged resources. IDisposable.Dispose is for releasing resources when you cannot wait for garbage collection.

@Jimmacle
Copy link

Jimmacle commented May 13, 2016

Regardless, if Dispose is not called on the MyVoxelGeometry instances (which the game is not programmed to do because you just implemented it) your changes have no effect.

EDIT: I actually looked at the FastResourceLock implementation and there's no need for MyVoxelGeometry to call its Dispose method at all because like Rynchodon said the destructor calls Dispose. What exactly does your change do that the game doesn't already do, then?

@malware-dev
Copy link

malware-dev commented May 14, 2016

@Rynchodon and @Jimmacle is right, there was never any need for this in the first place.

Just goes to prove that one should never trust tools implicitly. They can't think like a developer can, they can only give hints - and they are often wrong.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants