Federico Mena Quintero wrote:
<snip>
> > 3) sometimes it's just lack of C knowledge:
> >
> > if (p)
> > free(p);
> >
> > can be simply replaced by just:
> >
> > free(p);
>
> If p is a null pointer then "free (p)" may (and should!) crash. You
> are incorrect here.
Hm. According to most (all?) books I have and the manpages it is
perfectly legal to do a free(NULL). Anyhow, it's better to use g_free
instead to be on the save side.
<snip>
> > 2) we might break things that work
>
> This can be fixed by, you know, testing things after you change them :-)
Agree.
> > 3) we don't spend time on other fun things liking adding functionality.
>
> The GIMP does not need more functionality at this point; it needs to
> be checked for correctness.
I fully agree. I am just trying to make the point that by carefully
reviewing existing code we might find more bugs and in the meanwhile can
clean-up the code a bit. By cleaning up the code I don't mean extensive
re-writes or adding functionality, but just fixing obvious stuff.
(Although I realize that even fixing obvious stuff can result in bugs
;-) )
As an example of what I mean: somewhere in the xpm plugin (xpm.c) you
will find the following code:
/* get some basic stats about the image */
switch (gimp_drawable_type (drawable_ID))
{
case RGBA_IMAGE:
case INDEXEDA_IMAGE:
case GRAYA_IMAGE:
alpha = TRUE;
break;
case RGB_IMAGE:
case INDEXED_IMAGE:
case GRAY_IMAGE:
alpha = FALSE;
break;
default:
return FALSE;
}
switch (gimp_drawable_type (drawable_ID))
{
case GRAYA_IMAGE:
case GRAY_IMAGE:
color = FALSE;
break;
case RGBA_IMAGE:
case RGB_IMAGE:
case INDEXED_IMAGE:
case INDEXEDA_IMAGE:
color = TRUE;
break;
default:
return FALSE;
}
switch (gimp_drawable_type (drawable_ID))
{
case GRAYA_IMAGE:
case GRAY_IMAGE:
case RGBA_IMAGE:
case RGB_IMAGE:
indexed = FALSE;
break;
case INDEXED_IMAGE:
case INDEXEDA_IMAGE:
indexed = TRUE;
break;
default:
return FALSE;
}
This could be replaced by (sure hope I am not making mistakes now):
alpha = gimp_drawable_has_alpha (drawable_ID);
color = gimp_drawable_is_rgb (drawable_ID);
indexed = gimp_drawable_is_indexed (drawable_ID);
5 minutes work, no change in functionality, sourcecode a bit shorter
(and much more readable) and a few hundred bytes less objectcode.
Maurits
