[Freeciv-Dev] Re: PATCH: remove map_adjust_[xy] invocations from server (PR#989)

[Raimar Falke <hawk@xxxxxxxxxxxxxxxxxxxxxxx>]

On Fri, Oct 05, 2001 at 12:25:52PM -0400, Jason Dorje Short wrote:
> Raimar Falke wrote:
> > 
> > On Fri, Oct 05, 2001 at 01:43:57AM -0700, jdorje@xxxxxxxxxxxxxxxxxxxxx 
> > wrote:
> > > Here is a correction to the previous patch.
> > >
> > > Changes include:
> > >
> > > - The client code was cut out; that wasn't supposed to be there anyway.
> > >
> > > - A redundante is_real_tile check was removed.  In this case I have now
> > > simply replaced a logging-of-error with a simple assertion.  It's
> > > cleaner but slightly less safe in non-debug mode.
> > >
> > > Unless I'm badly mistaken this should be ready for inclusion.
> > >
> > 
> > >  
> > > /**************************************************************************
> > > +Returns TRUE iff the map position is normal.  "Normal" here means that it
> > > +is both a real/valid coordinate set and that the coordinates are in their
> > > +canonical/proper form.  In plain English: the coordinates must be on the
> > > +map.
> > > +**************************************************************************/
> > > +int is_normal_map_pos(int x, int y)
> > > +{
> > > +  return 0 <= y && y < map.ysize && 0 <= x && x < map.xsize;
> > 
> > This should be expresed in a way of normalize_map_pos. Or the other
> > way round.
> 
> I don't understand.  You mean this function should call
> normalize_map_pos?  That won't work; normalize_map_pos returns the
> realness of the coordinate.

int is_normal_map_pos(int x, int y)
{
  int tx=x,ty=y;
  if(!normalize_map_pos(&tx,&ty))
        return 0;
  if(x!=tx || y!=ty)
        return 0;
  return 1;
}

> Having normalize_map_pos call this function wouldn't work either

Yes correct. It doesn't work this way.

> , since normalize_map_pos must not only determine
> whether the value is real but which direction it is missing in (you
> could make the call, but you'd still have to check if it returned
> false).
> 
> > We may avoid the function call if is_normal_map_pos is a
> > macro. And with the RANGE_CHECK_0 from Ross there would also be no
> > double evaluation.
> 
> Well, whatever.  Should I add RANGE_CHECK_0 just for this patch? 
> Shouldn't that be its own patch?  Would you rather I propose that patch
> first?  Should I make this into a macro?  What is the consensus on any
> of these issues?

Good questions. Although I wrote something in another email I'm still
not sure about the CHECK_MAP_POS changes. CHECK_MAP_POS can and
probably will be implemented using is_normal_map_pos. However there is
a semantic difference between CHECK_MAP_POS and is_normal_map_pos:
 - CHECK_MAP_POS is for checking function arguments. CHECK_MAP_POS can
 and will be deactivated OR placed in the core access methods:
 map_get_tile and the other one or in map_index_checked.
 - is_normal_map_pos is for all "other" issues. This may include in
 the near future the changes you propose to savegame.c

Do people understand this? Is this ok?

If it is ok you can start with a plain is_normal_map_pos from above
(as a function) and I would then apply the CHECK_MAP_POS patch.

A map_index_checked or map_pos_to_index_checked (well more a
MAP_POS_TO_INDEX_CHECKED) patch is also possible.

The changes you have done in map_adjust-3.diff are partly
CHECK_MAP_POS and partly MAP_POS_TO_INDEX_CHECKED.

> > Mhhh since this obvious wrong: either iterate randomly or save the
> > value in another array.
> 
> It's wrong but it doesn't really matter; it smooths things out just
> fine.  If you really want I'll provide another patch that fixes the code
> to move values to another array.

A seperate patch would be nice.

> > > +  assert(is_real_tile(x, y));
> > > +  normalize_map_pos(&x, &y);
> > 
> > is_real_tile is unnecessary.
> 
> You mean I should use
> 
> is_real = normalize_map_pos(&x, &y);
> assert(is_real);
> 
> ?
> 
> That means a new variable declaration for every function, which is (IMO)
> not worth the time savings it gives.  Almost all existing code does it
> my way.

All such code is removed in the CHECK_MAP_POS patch. Silly me that I
haven't noticed this before.

        Raimar

-- 
 email: rf13@xxxxxxxxxxxxxxxxx
  Windows: From the people who brought you edlin...