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

[Jason Short <vze2zq63@xxxxxxxxxxx>]

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