KeenSoftwareHouse / SpaceEngineers

2.93k stars 896 forks source link

Resource leak patch #526

Open AlonzoTG opened 8 years ago

AlonzoTG commented 8 years ago

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.

malware-dev commented 8 years ago

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

Jimmacle commented 8 years ago

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 commented 8 years ago

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 commented 8 years ago

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 commented 8 years ago

@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.