Re: [PATCH] Scaffold -- some gnome-build leaks fix

On Thu, 2003-09-04 at 22:02, Rui Lopes wrote:
> Hi all,
> 
> Here are some gnome-build leaks fixes for am backend.
> 
> 
> I need you to look at my XXX/TODO comments and tell me what you think!

Looked them over.  A couple of comments:

You don't need the \n at the end of g_message/g_error/etc. because it
already puts it there.

info->build_dir = NULL; /* XXX: Why? */ - Always good to NULL out your
variables after freeing them because failing to do so may cause a
process to read invalid data and makes it easier to track down bugs.  I
always see a lot of assert(var!=NULL); but never
assert(var!="dfhfs#$%f");  Plus NULLing out a variable can't hurt.

/* XXX: don't we need to free warn? */ - I'm not totaly sure but my
guess is no.  build_msg prints out to the build window which I presume
takes ownership of the pointer.  It it were simply copying we wouldn't
need a strdup.  If you free'ed it you would crash the build window.  I
think the window treats each line as an object which can be clicked on
to bring you to the source line where the error or warning occured.

/* XXX: don't we need to free err? */ - again same as above

/* TODO: also report err->message using build_msg() */ - No, I don't
think that is right.  The build window is for errors with the build. 
g_error is for errors that realy should not occure and should be caught
during development.  Debug messages if you will.

 /* TODO: These paths should come from a configuration interface .. */ -
Yes, these should come from gconf.

/* XXX: This error shouldnt go to the interface using build_msg()? */
 g_warning ("Couldn't spawn %s\n", argv[0]); - Again this is not an
error with the build but rather a critical error.  It shouldn't happen
except in testing or in someones severly broken enviornment.

 /* XXX: gnu regex do not support utf8, we should use PCRE */ - What is
PCRE and is it a standard component.  We don't want to introduce
dependencies.  Besides why would one need utf8 support for parsing
sourcecode? I don't know of many languages that use special characters
out of the ASCII range and internationalized text is usualy taken care
of in the translation files.  

/* TODO: Get font description from config */ - Again this should come
from gconf

Hope that helps.

--
J5