Opened 12 years ago

Closed 12 years ago

Last modified 21 months ago

#8846 closed patch

Memory leak fixes

Reported by: SF/gamblore Owned by: fingolfin
Priority: normal Component: --Other--
Keywords: Cc:
Game:

Description

Memory leak fixes in

ScummEngine_v5 and ScummEngine_v7 /engine/scumm/scumm.cpp adlib Sound sound/softsynth/adlib.cpp Evaluator gui/eval.cpp Singletons common/singleton.h Graphics scaler graphics/scaler.{cpp,h} SDL Backend backend/platform/sdl/sdl.cpp SDL Graphics backend/platform/sdl/graphics.cpp scummvm_main base/main.cpp

Approximately 1MB in memory leaks plugged.

I am unsure as to if I am destroying the graphics scaler buffers too soon.

Ticket imported from: #1925352. Ticket imported from: patches/951.

Attachments (1)

memorypatches (3.8 KB ) - added by SF/gamblore 12 years ago.
Patches for all files listed

Download all attachments as: .zip

Change History (7)

by SF/gamblore, 12 years ago

Attachment: memorypatches added

Patches for all files listed

comment:1 by fingolfin, 12 years ago

Thanks, this is very much appreciated. However, instead/before just checking this in, I would like to get some things clarified:

* I am confused about the change to gui/eval.cpp. As it is, a HashMap *should* automatically free its content. Also, the ~Eval destructor should automatically call the destructors of all members. So, in theory, there should be no reason to call _strings.clear() in the ~Eval destructor. Your patch indicates that this is not true. If that is indeed the case, this hints at a deeper underlying problem, which should be identified and fixed properly once and for all. Instead of calling clear() manually everywhere.

* It is safe to call delete and free() on NULL pointers, so no need to check vars for being non-zero before deleting/freeing them

* You added a Singleton::destroy method, yet for the Config Manager (which is a singleton), you don't use that but rather call its destructor manually. Is that on purpose?

comment:2 by SF/gamblore, 12 years ago

Alright heres what I was thinking... or not thinking in that manner.

The gui/eval.cpp .clears() are not needed... not sure what i was thinking. I must have got confused between reset() and the deconstructor somehow.

Yes don't really know why I did it really.

Yes this was a mistake. ConfMan.destroy() works.

comment:3 by fingolfin, 12 years ago

No worries :). And ~Eval calls several clear() methods already, which are probably just as pointless, so no harm :) I just prefer to double check the real causes.

comment:4 by fingolfin, 12 years ago

Owner: set to fingolfin
Status: newclosed

comment:5 by fingolfin, 12 years ago

Commited, with some further tweaks (Singleton::destroy sets the singleton pointer to 0)

comment:6 by digitall, 21 months ago

Component: --Other--
Note: See TracTickets for help on using tickets.