Re: Code cleanup
Nick Lamb [EMAIL PROTECTED] writes: If p is a null pointer then "free (p)" may (and should!) crash. You are incorrect here. sigh You are completely wrong, see KR's treatment of ANSI C Sorry, my bad. I just checked the ISO standard and indeed, free(NULL) is safe. I guess that makes me an old fogey. Federico
Code cleanup
I just had a look at the code of some of the plug-ins and I noticed that there is often lot's of room for improvement: 1) some constructions are plain clumsy, for example somewhere I saw: for (k = 0; k bytes; k++) destline++; instead of simply: destline += bytes; 2) a lot of functionality has been added since the last update of some of the plug-ins. Result: duplicate functionality 3) sometimes it's just lack of C knowledge: if (p) free(p); can be simply replaced by just: free(p); No need for the test, unless performance is really critical and we expect to free a lot of NULL pointers. 4) some plugins have obvious memory leaks. So my question: is it worth the effort to carefully go through all the code (not only plug-ins) and clean things up? Advantages: 1) source code becomes smaller. Not a big deal with Gb harddisks, but nice for future maintenance. 2) object code becomes smaller. My initial estimate that for example most plug-ins can be easily reduced with 10 a 20 % without much effort. 3) during the process we might find further bugs. Disadvantages: 1) this will cost time/effort (I am willing to make my contribution). 2) we might break things that work 3) we don't spend time on other fun things liking adding functionality. Any thoughts/comments?
Re: Code cleanup
On Thursday, 31 Aug 2000, Maurits Rijk wrote: 3) sometimes it's just lack of C knowledge: if (p) free(p); can be simply replaced by just: free(p); I would not assume that it is safe to free() a NULL pointer in _all_ vendor's libc implementations. That's why there are things like g_free() that explicitly make these kind of guarantees. 4) some plugins have obvious memory leaks. Be careful - it is easy to "fix" a memory leak and introduce another bug where some contorted path through the code actually does use the memory. This has already happened a few times within the last year. So my question: is it worth the effort to carefully go through all the code (not only plug-ins) and clean things up? Yes, if done properly. However: 3) during the process we might find further bugs. Yes, but a code walk-through would have the same effect, without potentially adding more bugs: 2) we might break things that work This is the biggest danger right now. A freeze is a freeze! Austin
Re: Code cleanup
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
Re: Code cleanup
On Thu, Aug 31, 2000 at 03:48:37PM -0400, Federico Mena Quintero wrote: Maurits Rijk [EMAIL PROTECTED] writes: [ ... ] 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. [ ... ] I disagree with you there. Here's an excerpt from the free(3) man page: free() frees the memory space pointed to by ptr, which must have been returned by a previous call to malloc(), calloc() or realloc(). Otherwise, or if free(ptr) has already been called before, undefined behaviour occurs. If ptr is NULL, no operation is performed. It doesn't says: "If ptr is NULL, the program may crash". DindinX -- [EMAIL PROTECTED] Author of the French Book: Programmation Linux avec GTK+ A man with one watch knows what time it is. A man with two watches is never quite sure.
Re: Code cleanup
On Thu, Aug 31, 2000 at 09:22:56PM +0200, Mail Delivery System [EMAIL PROTECTED] wrote: if (p) free(p); I would not assume that it is safe to free() a NULL pointer in _all_ True. OTOH, this has been an internationally accepted standard since 11 years. The question is sometimes as to wether each and every programmer must be hurt by non-C-ism's in some vendors os's, rather than hurting the user of gimp(!) on a ten year old machine. ;- (I am not advocating to change g_free to free everywhere ;) 2) we might break things that work This is the biggest danger right now. A freeze is a freeze! a freeze? a freeze? where is a freeze? ;() -- -==- | ==-- _ | ---==---(_)__ __ __ Marc Lehmann +-- --==---/ / _ \/ // /\ \/ / [EMAIL PROTECTED] |e| -=/_/_//_/\_,_/ /_/\_\ XX11-RIPE --+ The choice of a GNU generation | |
Re: Code cleanup
On Thu, Aug 31, 2000 at 03:48:37PM -0400, Federico Mena Quintero wrote: If p is a null pointer then "free (p)" may (and should!) crash. You are incorrect here. sigh You are completely wrong, see KR's treatment of ANSI C Standard Library, Section B6 void free(void *p) free deallocates the space pointed to by p; it does nothing if p is NULL. p must be a pointer to space previously allocated by calloc, malloc, or realloc. Find me a system that doesn't pretend to implement ANSI C, ISO C or POSIX and I'll show you a system that won't build Gimp. Nick.
Re: Code cleanup
Maurits Rijk wrote: I just had a look at the code of some of the plug-ins and I noticed that there is often lot's of room for improvement: Yep. snipped... So my question: is it worth the effort to carefully go through all the code (not only plug-ins) and clean things up? Worth relates to whether a plug-in is worthy. Surely, the effort of a generally worthy plug-in deserves all the maintenance it can get. That is the idea behind the PLUGIN_MAINTAINERS file which Sven Neumann inaugurated on 4 January, 2000. Quite frankly, plug-ins compete for mindshare. Worthy plug-ins accrue individuals willing to maintain them. Maintainers shake out as much of the cruftiness that their time and talent allows and balance the advantages and disadvantages that Mr. Rijk advanced. The beauty of this is plain: the worthiness of a particular plug-in is not based on my humble opinion, nor yours, no anybody else's. It is simply based on the fact that it has accrued a maintainer, someone who cares enough about it to fight code rot. Now in the 240 days or so since PLUGIN_MAINTAINERS has been in the CVS tree, about fifty-five plugins appear to have garnered maintainers, out of a population of 165 or so. Of the two-thirds without regular maintainers, quite a few are like cacti, requiring little water and even less soil, they continue functioning with only occasional maintenance from bug fixers. Others are -- well, you can inspect the bug reports as well as I and correlate the unmaintained titles with outstanding reports. If, in the event that something like a 1.2 Official Release should crowd close upon us, and a motion is made to reduce the size of the distribution, I would claim that an unmaintained plug-in with outstanding bug reports is an ideal candidate to oust from the package. There would be unbiased and quantifiable reasons to do so. My two U. S. cents. Garry Osgood
