[Freeciv-Dev] Re: [PATCH] Map cleanups (PR#1208)

[Jason Short <vze2zq63@xxxxxxxxxxx>]

Ross W. Wetmore wrote:

> At 05:19 PM 02/01/09 -0500, Jason Short wrote:
>
> > Raimar Falke wrote:
> >
> >
> > > On Tue, Jan 08, 2002 at 03:41:24PM -0500, Jason Short wrote:
> > >
> > >
> > > > Raimar Falke wrote:
> > > >
> > > >
> > > > > On Tue, Jan 08, 2002 at 03:50:59AM -0500, Jason Short wrote:
> > > What do you want the sample construct to be exchanged with?
> > >
> > > CHECK_MAP_POS(x,y);
> > > normalize_map_pos(&x, &y);
> > > or
> > > normalize_map_pos(&x, &y);
> > > CHECK_MAP_POS(x,y);
> > > or
> > > CHECK_MAP_POS(x,y);
> > >
> > > The first two makes the non-NDEBUG case slower. And the last is a more
> > > stricter check than the sample construct.
> And when Jason changes CHECK_MAP_POS() to use normalize_map_pos() you
> can double the whole inefficiency program output with the first two ...
As I said, there should be no CHECK_MAP_POS in this construct.  This is 
a completely independent construct designed to deal with a different 
scenario.

> > > So you suggest something like: rename CHECK_MAP_POS to
> > > CHECK_FOR_NORMAL_MAP_POS, add CHECK_FOR_REAL_MAP_POS and
> > > CHECK_FOR_REAL_MAP_POS has the nice side-effect of making the map
> > > position normal if this is possible (real)?
> And, now for today's consistent naming contest ...
>
>
> About the only thing you are doing here is put back the calls of the
> form
>
>   assert(normalize_map_pos(&x,&y));
>
> that everyone found so distasteful to begin with, or maybe it will look
> like the next version
These calls aren't distasteful, they are wrong.  They cause the code to 
work fine in DEBUG and then break in NDEBUG.

>   is_real = normalize_map_pos();
>   assert(is_real);
>
> which is the code you just finished replacing with CHECK_MAP_POS() because
> this was inefficient and (horrors) had the side effect of fixing up those
> positions that can always be fixed for the soft unnormal condition even
> when asserts were off.
As I said, some of this code can be replaced with CHECK_MAP_POS.  Some 
of it cannot.  There are still ~30 constructs of this form, plus ~8 
similar constructs using MAPSTEP.

> Oops, sorry ... I forgot, no history.
>
>
> > I think CHECK_MAP_POS should be left as-is (at least in name), and a new 
> > function/macro normalize_real_map_pos introduced.  Note the cases in 
> > which these are used: CHECK_MAP_POS is used when we fully expect the 
> > position to be normal, and usually can't deal at all with it being 
> > unreal.  normalize_real_map_pos is used when we need a normalization but 
> > expect the position to be real (and can't or don't want to deal with the 
> > unreal case).
> But we don't want to just use normalize_map_pos() because then we would
> need to ignore the return value.
No, we would need to assert that the return value is TRUE, which is 
exactly what we do now.  This just simplifies the process.

> > CHECK_MAP_POS therefore has to choose between speed and safety - in the 
> > NDEBUG case, it may choose to do the normalization (safety) or not 
> > (speed).  Currently it goes for speed.
> But you have shown us your plan to fix this, right ...
Nothing is broken.


> > normalize_real_map_pos _must_ do a normalization, so there's no real 
> > choice there.  The only choice is whether we do a nearest_real_pos in 
> > the case of unreal positions, but this is just a hack to avoid a 
> > potential crash.
> Ahh, normalize_real_map_pos() is to be the new child-of-void_tile?
>
> This is the one that handles the unreal condition that no one quite
> understands (duh) and confuses with non-normal ambiguity, right?
No.  Your void-tile-phobia has blinded you to the truth: that in most 
cases, the ONLY way to deal with an unreal position is to point it at 
some valid tile.  This may give unpredictable behavior, but in the 
NDEBUG case it is probably better than aborting.  As long as we make 
sure to assert in the DEBUG case, it will not cause any new bugs and 
will allow existing bugs to not be fatal.

> > In a perfect world, it might not be, but freeciv has a number of places 
> > that pass around, for instance, goto directions for units.  Here we have 
> > the unit's original position and a direction.  We know the direction is 
> > valid, but we still must do the normalization anyway.
> But, but, but ... does this mean that MAP_STEP is now out of favour?
>
> This was the one that was supposed to take coordinates and direction and
> turn them into a nice safe new coordinate position?
I don't understand the question.  Why is MAP_STEP out of favor?


> Sorry, I still don't understand why we need a new name and definition
> for these constructs every other Tuesday :-). 
> It must mean you haven't quite figured it all out yet, right?
Though we all tend to behave otherwise, nobody has figured it all out.


> It is the loopy guess and eliminate iterative method of code devolution?
I prefer to think of it as incremental improvement.  Your corecleanups 
have taken a more structured approach, yet still your code constructs to 
handle these situations leave much to be desired.

> It keeps the patch count up to slow down pollution of the codebase from
> infidel heresies?
Smaller patches are easier to review and apply.  Addressing only one 
issue in a patch gives it an order-of-magnitude higher chance of being 
accepted.  You do the math...
jason