[Freeciv-Dev] Re: (PR#6941) Mission Orders

["Arnstein Lindgard" <a-l@xxxxxxx>]

<URL: http://rt.freeciv.org/Ticket/Display.html?id=6941 >

On Wed, 28 Jan 2004 09:37:21 -0800 Jason Short wrote:

> The patch doesn't apply cleanly:
> 
> 1 out of 34 hunks FAILED -- saving rejects to file client/control.c.rej

I can't reproduce.. even with the patch downloaded.

> - You shouldn't include common/packets_gen.* in the patch.  These just
> bloat it.  I usually rm common/packets_gen.* before making the patch.

Ok. Why not add them to the ignore file?

> 
> - The function order_is_action() is misleading.  Movement is an action too!

Yes. I can't think of a better name :) I'm sure it will come to me later.

> - I think the union shouldn't be declared globally.

The union is used in many other files. Of course we could simply skip
the union and use a plain integer, since that is part of C ..

> What about
> something like
> 
> struct unit_order {
>   enum orders_type type;
>   union {
>     enum unit_orders order;
>     enum unit_activity activity;
>     enum unit_actions action;
>     enum diplomat_actions diplomat;
>   };
>   enum direction8 dir;
> }

That's specific to gnu C. The union needs a name to work with C99.
"type" is server -> client information for sprite purposes, I don't
think the client will ever need to send that.

> - Finally, I think this code needs a lot of design review.  I'm not sure
> that the interface (common/unit.h) is best.  Can we separate the common
> and server parts and consider them first?  The client interface can be
> added on afterward.

Sure, but only when you look at the client code will you see that
the union is referenced in other places.


Arnstein