[Freeciv-Dev] Re: Cleaning up gotohand.c (PR#1068)

[Raimar Falke <hawk@xxxxxxxxxxxxxxxxxxxxxxx>]

> On Tue, Nov 20, 2001 at 03:57:43PM +0000, Gregory Berkolaiko wrote:
> And here comes the patch itself....

> * removed "silence compiler warning" line because there is no compiler 
> warning anymore

The sun compiler is known to be more strict than gcc.

> In goto_zoc_ok:
> * as was proposed in the comment, changed the code to take into account
> where we _really_ came from, not to guess the direction from dir_ok
> * changed the comment accordingly

Please make a seperate patch.

> In find_a_direction:
> * the previous handling of trireme was pathetic.  changed it to use
> is_coast_seen.  NB: it does not mean that triremes are safe: if
> find_a_direction is given an unsafe destination it is unlikely to help.

Please make a seperate patch.

> -  /* FIXME: Should this apply only to F_CITIES units? -- jjm */
> +  /* FIXME: Should this apply only to F_CITIES units? -- jjm 
> +   * I don't think so, why should engineers go too far? -- GB */
>    if (punit
>        && unit_flag(punit, F_SETTLERS)

There is a comment refering to F_CITIES. You change it. But the code
two lines below referer to F_SETTLERS.

>          if (map_get_terrain(x1, y1) == T_OCEAN) {
> +       /* attempting to walk on water.  Can do if there is a boat there */
>            if (punit && ground_unit_transporter_capacity(x1, y1, pplayer) > 0)

Nit-picking: Either you write sentences: "The unit attempts to walk on
water. Can do if there is a boat there." or prompts "attempting to
walk on water; can do if there is a boat there".

> -       /* NOT c = 1 (Syela) [why not? - Thue] */
> -       move_cost = (ptile->move_cost[dir] ? SINGLE_MOVE : 0);
> +       /* NOT move_cost = MOVE_COST_ROAD (Syela) [why not? - Thue] 
> +        * possibly because if an igter unit reuses warmap of a city 
> +        * (see generate_warmap), it won't be THAT much different 
> +        * from the correct warmap.  For consistency we should have 
> +        * SINGLE_MOVE for disembarking igter unit too -- GB */

No. Please don't write novels in comments. Especially if they start
with "possibly".

> +       /* normal move; the most movepoints a unit can spend on a move 
> +        * is his move_rate */ 

AFAIK it is "its" and not "his".

> +    /* Checking for warmap in existence.  
> +     * If the previous warmap was for a city and our unit is 
> +     * in that city, use city's warmap.

> +     * That sounds quite wrong given that there are different 
> +     * types of units and only one way to generate city's warmap. */

Does this mean "That sounds wrong at first sight BUT is correct
because..." or "That is wrong"?

>  static int goto_zoc_ok(struct unit *punit, int src_x, int src_y,
> -                    int dest_x, int dest_y)
> +                    int dest_x, int dest_y, unsigned char came_from)

Directions are int.

Summary: can you split this into "comment changes", "changes which
doesn't affect the behavior" and the "behavior changes"?

Alternatively I can also extract all things which I think are correct
based on a first scan.

        Raimar

-- 
 email: rf13@xxxxxxxxxxxxxxxxx
  Two OS engineers facing a petri net chart:
        "dead lock in four moves!"