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

[Gregory Berkolaiko <gberkolaiko@xxxxxxxxxxx>]

 --- Raimar Falke <hawk@xxxxxxxxxxxxxxxxxxxxxxx> wrote: 
> > On Tue, Nov 20, 2001 at 03:57:43PM +0000, Gregory Berkolaiko wrote:
> 
> > * removed "silence compiler warning" line because there is no
> compiler warning anymore
> 
> The sun compiler is known to be more strict than gcc.

ok

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

ok

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

ok

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

JJM says: I think the below should only apply to F_CITIES units, not to
F_SETTLERS.
I say: I think it should apply to F_SETTLERS
Now I think it should apply to both F_SETTLERS and F_CITIES (I studied
the distinction a bit more).

I can remove my comment if you want

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

ok, Mr.Teacher ;)

> > -     /* 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".

This is a question ("why not?") to which no one so far could give _any_
answer.  I formulate a hypothesis which I think should be mentioned
somewhere.  If not code than where?

As for "possibly", I didn't write the code, so I don't know for sure.
Sorry.

If you have a better idea as to why an IGTER move costs SINGLE_MOVE and
not MOVE_COST_ROAD on the warmap, please enlighten us.

> > +     /* normal move; the most movepoints a unit can spend on a move 
> > +      * is his move_rate */ 
> 
> AFAIK it is "its" and not "his".

you are right

> > +    /* 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"?

It is "It sounds wrong to me".
I will fix it.

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

this is not a direction.
here you plug an element from
unsigned char *vector[MAP_MAX_WIDTH];
from struct move_cost_map.

why are directions int?

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

I will do so.

G.


__________________________________________________
Do You Yahoo!?
Everything you'll ever need on one web page from News and Sport to Email and 
Music Charts
http://uk.my.yahoo.com