[MPlayer-dev-eng] [PATCH] Free GUI config

[Reimar Döffinger]

On Fri, Apr 29, 2011 at 10:45:26AM +0200, Ingo Brückl wrote:
> It seems that the times when I started programing are far too far away. I'm
> pretty sure I learned to properly release all memory I'm allocating.

That is in principle still a good rule, it eases debugging memory
leaks with valgrind. It also makes it easier to restructure code,
e.g. if you don't do that going from
int main() {
  malloc_something
  process_something
}

to

int main() {
  loop {
    malloc_something
    process_something
  }
}

will introduce a real memleak, and you might realize only now that your
design does not allow you to free properly. So going with that rule
helps you avoid digging yourself into a hole you might have trouble
finding out of.

> I am
> stunned to come to know that it will be released anyway when the process
> exits. (Seems that I am lacking some modern programing knowledge.)

All operating-system managed resources will be freed on process exit.
That means mostly memory and file descriptors.
That also means you have to be extra-careful about properly handling
any resources your operating-system has no idea of.
That is rare since normally you'd try to write a driver or similar to
make the OS aware of it and handle it.
But if you had a compute cluster and a program that allocates
machines to help it with its tasks you'd certainly want to make sure that
the program releases those machines again if you e.g. abort it.
Though since that's usually still brittle that's usually managed in
a more fail-safe fashion anyway.
(Note that this has some similarity with Java where the bar was moved
further so explicit frees are not even necessary at runtime for memory.
Unfortunately here resources like file handles didn't quite make it
and still need explicit handling)

> >> Shouldn't there be a m_config_free() after GUI's m_config_new()
> >> and shouldn't it be done right before exit, i.e. before gmplayer ends and
> >> options are no longer needed?
> 
> > Well, I am asking you why you think it should.
> 
> At least for the same reason why there is a m_config_free(mconfig) around.
> (It should be pointless then as well.)

The reason for that is that the valgrind reports were quite unreadable
so it was added to make fixing mem-leaks easier and it was little
and clean code.
There are no clear rules though, it is a matter of preference, how
much code/uglyness is it worth to "properly free" something just
for the sake of it?

> >> If an error occurs before initialization is finished,
> >> guiDone() will never get called and thus the freeing will never be performed.
> >> In mplayer.c (with the CONFIG_GUI) it will be performed in any case.
> 
> > When and why would that actually matter?
> 
> Although it no longer matters: At every exit_player...() between lines 2766
> and 3024, including all aborts during GUI initialization (no skin, no draw
> buffer).

I think it would be reasonable to put it in guiDone with a comment
that it won't be quite correct in some corner-cases.
Like that you still have the advantage of doing it mostly right,
valgrind not complaining in basically every use-case where you'd
reasonable want to use it to search for memleaks and not cluttering
the core code with Gui-code which has its issues (including making
both Gui and core code harder to understand).