[Freeciv-Dev] Re: [UPDATE] Corecleanup_08 patch to cvs-Sep28

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

At 09:38 AM 01/10/05 +0200, Raimar Falke wrote:
>On Thu, Oct 04, 2001 at 10:10:19PM -0400, Ross W. Wetmore wrote:
[...]
>
>Short comment on this: the patch includes reformatting like:
> -  return i / 2;
> +
> +  return i/2;
>which are useless without a strict coding style (which we still lack).

Actually, these are often the original CVS where I haven't picked up 
all the patch bits that added spaces everywhere. In patches for CVS I
have usually made a sanity run to remove redundant cosmetic stuff unless 
it is too endemic and/or I really think CVS should be "improved".

There may be some in the real patches, but it will be minimal.

Can we stop Gaute from doing this unnecessarily, too :-?

>It includes pieces like:
> -  map_city_radius_iterate(src_x, src_y, x1, y1) {
> +  city_map_checked_iterate(src_x, src_y, cx, cy, x1, y1) {
>      struct city *pcity = map_get_city(x1, y1);
>      if (pcity) {
>        update_city_tile_status_map(pcity, src_x, src_y);
>      }
> -  } map_city_radius_iterate_end;
> +  } city_map_checked_iterate_end;
>which are ok and I would like to get as a seperate patch.

There are I think 3 places in unittools. And it was part of the cleanup
to get rid of all the duplicate flavours of city_map iteration along
with the cases where the checked flavour was useful.

If you want, I can do the city.h consolidation and anything left in CVS 
that doesn't conform to the final set. Look at city.h and let me know
whether you want to get this all out of the way.

The simple renames were in corecleanup_07d, it was corecleanup_07h,i
that did the final parts of the job if you want a more concise handle
on the set of things to look at (of course understanding that not
everything may be quite the same now, but the stuff in corecleanup_08
is current). 

If you have any major changes to this let me know. Or if you are
happy enough with them, I'll package up the set for a proper review
cycle. The total will probably just over 500 lines of diff.

>It includes pieces like:
> -  punit->x = map_adjust_x(x); /* was = x, caused segfaults -- Syela */
> -  punit->y=y;
> -  if (y < 0 || y >= map.ysize) {
> +  punit->x = x;
> +  punit->y = y;
> +  if( !normalize_map_pos(&punit->x, &punit->y) ) {
>which are also ok.
>
>It includes code as:
> -          ACTIVITY_FORTIFIED and the unit has no chance of moving anyway */
> -       handle_unit_activity_request(bodyguard, ACTIVITY_IDLE);
> +          ACTIVITY_FORTIFIED and the unit has no chance of moving anyway
> +       handle_unit_activity_request(bodyguard, ACTIVITY_IDLE); */
>and
> +    pplayer->ai.expand +=
> +        (myrand(pplayer->ai.expand)-myrand(pplayer->ai.expand))/5;
>which will not be applied without a statement of motivation.

This is part of the AI stuff. The first is the response to the FIXME a
line or so above if you look at patched code.

The second tries to use the only real "personality" variable in the whole
server AI to provide per civ difference in the weight they give to 
procreation. Instead of 100% (no variation), it sets per civ random
value from 80 to 120. Smaller values kick one out of smallpox mode to 
vertical growth faster.

As a general I wouldn't recommend taking any of the AI stuff just yet. 
But if you want to try the expand one it doesn't really hurt.

>And there is also code like:
>        *canvas_x = map_x-map_view_x0;
> -    else if(map_x < map_adjust_x(map_view_x0+map_canvas_store_twidth))
> +    else if(map_x < map_wrap_x(map_view_x0+map_canvas_store_twidth))
>        *canvas_x = map_x+map.xsize-map_view_x0;
>
>which deals with the wrapping types (which is ok but such changes are
>buried under the other changes)

This is part of removing map_adjust. In some places the code really does
want to clip (i.e nearest_map_pos), but note it is pretty much confined to
client/GUI code, and should not be confused with doing bad things to
gaming coordinates as broken fixes for not understanding normalization, 
hence the function name is more apt.

If you and/or Jason agree, I can do all the map_adjust removal as well.

There are also a fair number of places where (x+1,y-1) and such are used
that would complete the corecleanups for most of the things needed to
have it all go through normalize_map_pos, mapstep or iterate. I think
I have all the +/-1, but there may be a +/-3 or something odd that I
haven't picked up via grep or triggered in testing yet.

This can be part of this or done as a separate patch if you want. Note
there is a fair bit of this in tilespec.c if you want to see what the
basic flavour is all about.

>       Raimar
>
>-- 
> email: rf13@xxxxxxxxxxxxxxxxx
> "Are you saying that you actually used the Classpath Java AWT classes in 
>  addition to the GTK peers and got them to display something?
>  Wow.  That's way better than I did and I wrote the code!"
>    -- Aaron M. Renn in the classpath mailing list