[Freeciv-Dev] (PR#5082) [PATCH] packet_short_unit

["Jason Short" <jdorje@xxxxxxxxxxxxxxxxxxxxx>]

[marko.lindqvist@xxxxxxxxxxx - Sat Aug 16 20:53:35 2003]:

>  This version
>  - Applies to latest CVS
>  - Compiles
>  - Minimally tested
> 
>  Second patch is for removing capability checks (to be applied when
> capstr cleared).

- In do_move_unit(), the name tempunit is confusing.  It needs to be
more self-explanatory...perhaps target_unit?  The same applies to
handle_unit_packet_common.

- Initializing empty values in unpackage_shore_unit is quite
unnecessary.  This is already done by create_unit_virtual.  If there are
any fields left out it's a bug in that function.

- Patch doesn't apply cleanly to civclient.c.  For this reason I haven't
tested it.

- Allocation and deallocation of "tempunit" is done inconsistently -
it's very hard to tell if this is correct.  The same functions that
calls unpackage_*** should call free() on the unit afterwards.  If
handle_unit_packet_common() needs to allocate memory it should do that
itself (punit = malloc(sizeof(*punit)); *punit = *tempunit;).

- 

  /* Special case for a diplomat/spy investigating a city:
     The investigator needs to know the supported and present
     units of a city, whether or not they are fogged. So, we
     send a list of them all before sending the city info. */

Is this really needed for both packet_short_unit and packet_unit?  The
info isn't send with packet_unit, so I suspect not.  If so it should be
separated into a helper function.

- Are you sure we don't need a manditory capability?  I think
fundamentally we do: the client could "cheat" by telling the server it
doesn't support packet_short_unit, and thus get more information from
the server.  Since new manditory capabilities are added so often
recently this is not a big deal.  Note that this is the equivalent of
"removing" the capability, allowing a fair amount of code to be removed
from the patch.

- There are lots of style issues.  Any line which you change should be
given consistent style (see the style guidelines).  Also instead of
commenting out multiple lines of code you should put #if 0...#endif
around them.

In general, it looks good - very promising.

jason