[Freeciv-Dev] Re: [Patch] General cleanup (splint)

[Raimar Falke <hawk@xxxxxxxxxxxxxxxxxxxxxxx>]

On Mon, Feb 04, 2002 at 01:08:58AM -0500, Jason Short wrote:
> Mike Kaufman wrote:
> 
> > On Sun, Feb 03, 2002 at 11:39:03PM +0100, Raimar Falke wrote:
> > 
> >>I didn't want to review another patch so I try something new: splint
> >>(www.splint.org). If the weak mode is restricted further the number of
> >>warnings went down to something manageable. The attached patch is the
> >>result to fix these:
> >> - unused return values: either check the result (fwrite), replace if
> >> with something other (client/options.c) or ignore the value
> >> - incomplete struct/array initializing
> >> - empty if block (ai/aicity.c)
> >>
> >>Discoveries:
> >> - ugly things like
> >>-GtkWidget *input_dialog_create(GtkWidget *parent, char *dialogname,
> >>-                          char *text, char *postinputtest,
> >>-                          void *ok_callback, gpointer ok_cli_data,
> >>-                          void *cancel_callback, gpointer cancel_cli_data)
> >>+GtkWidget *input_dialog_create(GtkWidget * parent, char *dialogname,
> >>+                              char *text, char *postinputtest,
> >>+                              GtkSignalFunc ok_callback,
> >>+                              gpointer ok_cli_data,
> >>+                              GtkSignalFunc cancel_callback,
> >>+                              gpointer cancel_cli_data)
> >>
> >> - nobody is interested in the return values of adjust_workers,
> >> key_city_workers, alloc_nations, do_unit_goto
> >> - nobody outside client/gui-gtk/mapctrl.c is interested in adjust_workers
> >> - common/ioz.c was really wrong all the time. Why hasn't anybody noticed 
> >> this?
> >> - send_packet_attribute_chunk was really wrong
> >> - nobody uses the return value of do_unit_goto. The return value has
> >> been introduced 2001/10/04. So dead code was added.
> >> - gui-xaw/menu.h:enum MenuID and gui-gtk/menu.c:enum MenuID look identical
> >>
> >>IMHO this tool is nice and I will try it a second this if these
> >>changes are in.
> >>
> >  
> > I think that this is a nice tool certainly, but I really dislike the
> > patch. My main problem is the 450-odd additions of (void) to functions.
> > Entirely unncessary in my opinion (granted, I did not look at every one,
> > there could be one or two that are justified)
> 
> I partially disagree.  In some cases, a return value on a function is 
> really not meant to be ignored, and ignoring it is a potentially large 
> bug.  

> This is often done with normalize_map_pos(), for instance. 

Interestingly I haven't found such a case.

> Forcing the cast to (void) in this case makes the author think about it 
> a second time before doing it, and makes it clear to the reader that 
> that's what was intended.
> 
> On the other hand, it _is_ a lot of extra text.
> 
> 
> > I would like to take another look without these changes, but the other
> > changes _appear_ fine.
> 
> Yes, everything looks like no-op changes although I have not looked all 
> _that_ closely.

        Raimar

-- 
 email: rf13@xxxxxxxxxxxxxxxxx
 "Life is too short for reboots."