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

[Jeroen Zwartepoorte <jeroen xs4all nl>]

Hi guys,

Rui: thanks for the patch :)

Comments below.

On Fri, 2003-09-05 at 05: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.
> 
> 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.

You get all kinds of weird crashes when you reference a variable which
has been freed, but not "reset" to NULL. This is generally a good
practice.

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

Yeah, this is on my TODO list. I want to be able to have Run & Build
configurations in gnome-build. This basically comes down to the
following: a user can define a new Run or Build configurations, specify
which prefixes he wants to use, which parameters etc. This involves some
heavy hacking in gnome-build.

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

GNU regex _does_ support UTF-8, as of glibc 2.3 iirc. Otherwise
highlighting of UTF-8 files in GtkSourceView wouldn't work either (try
opening an XML file with UTF-8 text (schemas file for example)).

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

The gnome-build plugin in scaffold will eventually add a preference page
to the scaffold preference dialog. Things like the font that's used for
the build output can be configured here. The actual pref will indeed be
stored in gconf.

Rui: i'll try and get your patches reviewed/committed by the end of the
weekend.

Regards,

Jeroen