[Freeciv-Dev] Re: bug with initialization of goto_dest_[xy]

[Raimar Falke <hawk@xxxxxxxxxxxxxxxxxxxxxxx>]

On Wed, Jan 02, 2002 at 02:16:11AM -0500, Jason Short wrote:
> The values punit->goto_dest_x and punit->goto_dest_y are never 
> "initialized" when a unit is created, and there's no way to indicate 
> that the unit has no goto destination.  Rather, on initialization the 
> values just get set to (0,0) and later may (perhaps?) be reset to this 
> later if the goto destination is removed.
> 
> Code that uses the goto values generally compares them to something, or 
> looks to see what's there (a city, an enemy unit, etc.), and ignores the 
> goto if not.  Since the tile (0,0) is generally unusable, this is fairly 
> safe, although it is far from "correct".
> 
> Under a non-rectangular topology, this hack leads to a bug.  For 
> instance, under an iso-rectanguler topology (0,0) is generally not a 
> "normal" map position, so CHECK_MAP_POS will fail and there will be a 
> segfault.  So, one solution is to hack the hack so that we call 
> is_normal_map_pos to check for this case before using the coordinates 
> (this is only necessary in a few cases).  As long as (0,0) always 
> remains an unusable OR non-normal position, the hack should continue to 
> work.  The attached patch makes this change; it is the simplest one 
> possible AFAICT.
> 
> The "correct" solution would IMO be to add another piece of data to 
> track whether the goto_dest is valid at all.  I can think of two ways to 
> do this: (1) make the goto_dest a struct map_position pointer, which is 
> set at NULL when the goto_dest is not in use and malloc'ed as necessary, 
> or (2) create a separate field goto_dest_is_used (or the like) that can 
> store the validity information for the position.
> 
> I really think we should go for the correct solution here.  However, it 
> will be a non-trivial task, so I've only provided this small fix for now.
> 
> Note, all of this is closely tied to goto_is_sane() in gotohand.c. 
> However, goto_is_sane will segfault if given non-normal coordinates, so 
> even if we use that function to check coordinates we'll have to fix it up.
> 
> Note also, Ross's corecleanups attempt to fix these pieces of code by 
> calling normalize_map_pos() on the goto_dest map position.  This is a 
> bug (and a nasty, one, too!), since it may wrap the invalid position to 
> a valid one, thus causing the goto to be set to some random position 
> somewhere on the map!

Since we can assume that the goto destination is either not set or
normal I propose the following:
 - add a new function unit_has_a_valid_goto_destination which is just
 a is_normal_map_pos
 - set the initial values to values which are granted to be always
 un-normal: -1, -1 for example.

        Raimar

-- 
 email: rf13@xxxxxxxxxxxxxxxxx
 "Transported to a surreal landscape, a young girl kills the first woman
  she meets and then teams up with three complete strangers to kill again."
    -- TV listing for the Wizard of Oz in the Marin Independent Journal