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

[Jason Short <vze2zq63@xxxxxxxxxxx>]

Raimar Falke wrote:

> 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.
And that's a good thing :-).  Most such places have been converted to
  is_real = normalize_map_pos(...);
  assert(is_real);


There are a few questionable places remaining:

server/gamehand.c:115 - here the code isn't buggy, but it's 
_potentially_ buggy since it assumes normalize_map_pos leaves the values 
unchanged in the unreal case (which is currently not guaranteed...Ross 
wants it to be...I think calling code shouldn't assume this even if it 
is...etc.).  It also results in a spurious is_real_tile call, and makes 
the logic of the code harder to follow.
everywhere - the code that does assert(is_real_tile(...)) before calling 
normalize_map_pos(...) isn't buggy in any way, but also results in a 
spurious is_real_tile call.
It's odd splint wouldn't catch these cases, but perhaps I don't entirely 
understand what error conditions it's looking for...

For the most part, though, the tendency to make this mistake comes in 
new patches.  For instance it is done in the most recent borders patch 
(a major bug), and Ross does it in his corecleanups (generally in places 
where it's best just to assert(is_real) on the result).
jason