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

[Mike Kaufman <mkaufman@xxxxxxxxxxxxxx>]

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

yes, this is my point. In other cases---and in the majority of these
cases---the return value is nice if you need it, but usually not meant
to be used. For example, the string handling functions, it is certainly
not a bug to not use the return value. Another are gtk signal functions.
Sure, if you _need_ to be able to block a certain signal, _and_ other
signals use the same callback, then you have to fetch the signal id from
the return value, but otherwise it's just the addition of a lot of text
as you say. Even create_unit(), which is a borderline case I think,
doesn't need the explicit cast. (but I would compromise on this case...)

You're right: if a function's normal use is by way of the return value,
then if the author is doing something clever without it, then the author
should show the explicit cast. 

You're also right: I do think that splint should be used to find places
where the return value should be used but isn't. This is a bug. The
cases I mention are not. Don't touch 'em.

-mike

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