[Freeciv-Dev] Re: (PR#6259) iso-map versions of native<->map pos conversion macros

["Jason Short" <jdorje@xxxxxxxxxxxxxxxxxxxxx>]

rwetmore@xxxxxxxxxxxx wrote:
> Below is the code from the corecleanups, which can be seen to pretty much
> validate Jason's macros below. It has been operational for a long time.
> In general, the code below is both operationally correct and reasonably
> well implemented.
> 
> 
> There are some minor nits that might be incorporrated to improve things.
> 
> First as the CC comments state, there is a potential division issue with
> transforming negative values that one can get around by using the shift
> operator for an integer divide by 2. This guarantees that no matter what
> way one sets interger roundoff this code continues to do the right thing.
> Otherwise, one has built in an unnecessary, and fairly severe constraint
> on the mathematical range of applicability of the APIs.

I'm fairly sure the current implementation (taken from the corecleanups) 
will work for all positions (including negative ones).

> The second is a minor performance nit in map_to_native_pos(). Basically
> one can remove the extra "2 * " for the map_x by moving it outside the
> brackets. Since these coordinate calls happen a lot, this is a worthwhile
> percentage improvement in the performance. Anyway, there is absolutely no
> reason to not do it efficiently.
> 
>  > +      *(pnat_x) = (2 * (map_x) - *(pnat_y) - (*(pnat_y) & 1)) / 2)        
>   \

True enough.  Feel free to provide a patch, preferably with some numbers 
to show what improvement it yields.

> The last issue is one of the ongoing problems with people "seasoning" every
> codepath with paranoia checks for the mistakes that were introduced a number
> of years back in handling normalization.

But native_to_map_pos and map_to_native_pos have no such checks.  So I'm 
not sure what you're referring to.

jason