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

[Rui Lopes <rui ruilopes com>]

Hi John,

On Sex, 2003-09-05 at 04:07, John (J5) Palmieri wrote: 
> 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.

Oh!  I fixed my patch.  I will post it to gnome bugs.


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

Yes, I known all that;  I should have noted that we don't need that
NULLing because in the next line we are affecting it to a sane value. :)



> /* 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

Yep you are right, currently it does not need to free it, but after
reading the relevant gnome-build frontend/backend iteration, I think we
really need to make the free's needed, since we can have several
callbacks registered on the backend, so the structure passed should be
owned by the backend (as the data send in build_start/end/output
messages), anyone who needs it should do a copy.

The relevant code for the callback is in file:
src/gbf/gbf-build-info.c::build_callback(..)

It's okay for me to do these changes?



> /* 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.

Humm, okay, I will remove that TODO.

These errors should go up (they can happen outside development cycle). 
And g_error is too drastic for this, a g_warning is sufficient, because
maybe the user can fix the problem.


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

Agreed :)

> /* 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.

www.pcre.org.  its a fairly used lib, and it also has a POSIX interface.


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

Humm, I can create python files with UTF-8 inside.

And since glib works with utf8 we need a utf8 regex lib.  but I guess we
don't need PCRE if gnuregex is really UTF-8 in FreeBSD/Linux..



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

Yep, the TODO is just to make a hard note on that. heh 
> Hope that helps.

Yes, it helped.  Thanks! :)



Regards,
Rui Lopes