[Freeciv-Dev] Re: Remove map_adjust_[xy] invocations from server (PR#1003)

[Raimar Falke <hawk@xxxxxxxxxxxxxxxxxxxxxxx>]

On Tue, Oct 23, 2001 at 03:53:18AM +0100, Gaute B Strokkenes wrote:
> On Mon, 22 Oct 2001, hawk@xxxxxxxxxxxxxxxxxxxxxxx wrote:
> 
> > I agree to this solution for all positions are at some time used as
> > arguments to map_get_tile and co.  The question is: what happends to
> > the rest? 
> 
> > Either we manually go through the code and set
> > assert(is_normal_map_pos()) (which is error prone and not robust)
> 
> Or we add the macro that we are going to stick inside map_inx().
> 
> I'm not sure why you think this is error prone and not robust; can you
> elaborate?  It's certainly not any worse than "the big hammer"
> mentioned below.

We have to go through the code and remove is_real_tile,
normalize_map_pos and is_normal_map_pos. Everything is fine if the
positions these methods work on are used in the function as an
arguement to some access method. What happens if this isn't the case?

> I think it is useful to take a step back and consider how likely such
> mistakes are going to be.  Converting manual loops to _iterate macros
> is fairly easy to do, and it is also easy to spot most of the other
> mistakes that lead to this situation.
> 

> It would be a good idea to add those checks in places where it is
> obvious that a check in map_inx() is not enough, such as same_pos().

This is my first suggestion.

> I would not spend too much time and effort trying to make 100% sure
> that we have found all the places where we might be able to spot such
> a mistake).
> 
> > OR we ignore this case 
> 
> > OR we take out the big hammer and insert assert(is_normal_map_pos())
> > into almost every function and disable it by default (because this
> > will have a performance impact).
> 
> The idea is to disable it by default, and let people turn it back on
> when they make changes to iteration macros etc. that need testing of
> this sort.  Since that is a distinct minority of the time, we do not
> need to care that much about how much time it takes.  I don't think it
> is going to take that much more time than the current approach (always
> check, always fix).

I would vote for the ignore the case. We remove all "illegal"
is_real_tile, normalize_map_pos and is_normal_map_pos. If a map
position is unreal or no-normal and isn't catched it is either:
 - changed by another bug or
 - normalized before it reached an access method or
 - not used at all or
 - used for an outgoing packet.

For the last case (and also on general) I would wish a more strict
testing of map positions which came from packets (server rejects,
client aborts).

        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