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

["Ross W. Wetmore" <rwetmore@xxxxxxxxxxxx>]

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

>>> > This is a optimization-only thing? How much percent of the runtime is
>>> > used for normalize_map_pos if CHECK_MAP_POS is disabled?

Better have the numbers to make sure the code doesn't increase efficiency
and ruin the whole program :-)

>>>I'm not concerned about the optimization, but about consistency,
>>>readability, and correctness.  Making this a macro saves about 32x2
>>>lines of code overall, and unifies the structures into one
>>>sure-to-be-correct one.

Ahhh, good back to sanity ... 

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

  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.

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.

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

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

>Both should do full debugging checks; i.e. assertions that the expected 
>conditions are met.

Of course, must keep the targets up ...

>Now, you might wonder why normalize_real_map_pos is necessary at all. 

No, you make perfectly consistent nonsense ... 

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

>jason

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?

It is the loopy guess and eliminate iterative method of code devolution?

It keeps the patch count up to slow down pollution of the codebase from
infidel heresies?

Cheers,
RossW
=====