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

["Marko Lindqvist" <marko.lindqvist@xxxxxxxxxxx>]

Jason Short wrote:

> [marko.lindqvist@xxxxxxxxxxx - Sat Aug 16 20:53:35 2003]:
> 
> 
>> This version
>> - Applies to latest CVS

- Applies to CVS snapshot Sep-05

>> - Compiles
>> - Minimally tested

- This is still true

>>
>> Second patch is for removing capability checks (to be applied when
>>capstr cleared).

  Patches are now merged into one and mandatory capability added

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

  In do_move_unit() 'target_unit' is used. In 
handle_unit_packet_common(), 'packet_unit' used.

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

  Removed initialization from unpackage_short_unit().

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

  Changed that way. I'm still not 100% happy with having malloc() deep 
in create_unit_virtual() and free() at handle_packet_unit_info() or 
handle_packet_short_unit().

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

  Back then freeciv policy was to avoid mandatory capabilities at all 
costs. Like I already mentioned, in this version capability removal 
patch is merged to it and mandatory capability is added.

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

  I read patch fast through and any '+' lines seem to fulfill freeciv 
style guide. I saw no outcommented code lines, but they probably went 
away when I removed that capability stuff.


  - Caz

psu_full.diff.gz
Description: psu_full.diff.gz