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

[Raimar Falke <hawk@xxxxxxxxxxxxxxxxxxxxxxx>]

On Thu, Oct 04, 2001 at 10:10:19PM -0400, Ross W. Wetmore wrote:
> I understand your hope that things might be easier to manage in smaller
> chunks, but I think that you are fooling yourself. The review is 
> probably actually easier working with a clean consistent version than
> something that is a bastard amalgamation of half-hearted and partial
> changes. 
> 
> It is also easier to just look at an updated snapshot vs a current
> CVS in side-by-side views for those things you are interested in, as 
> opposed to reading diffs.
> 
> Diffs in themselves are not that readable, and you will have a lot of
> difficulty using/applying random partial diffs without the supporting 
> changes. Or I will have a lot of effort redoing changes to fit with a 
> broken CVS and all possible orders that you might decide to apply them 
> if the splits are into Raimar's 500 line limits - this is just nonsense
> make work designed to obstruct and not advance Freeciv development, and
> there are a lot of similar current practices :-).
> 
> It is not productive to have to interpret, deal with or test each 
> artifical division of a consistent set of changes, with all the extra 
> hacks needed to make it work with broken code that is being fixed by 
> a whole series. Certainly not for review and basic understanding which 
> is where the process is usually blocked.
> 
> At least with the tested working snapshot, you can play and experiment
> in a consistent environment. 
> 
> Whereas, in your case, if you try to play with just the AI code applied
> to a current CVS, then you are neither looking at it in the environment
> it was built and tested in, nor are you likely reviewing the changes
> but just rediscovering already found bugs and problems :-).
> 
> The mess of partial changes that is current CVS attests to the viability
> of doing a large standardization effort in random bits and pieces! 
> 
> And frankly, I really *am* tired of doing yet-another-split everytime 
> someone wants to make a spurious cosmetic change BEFORE they actually 
> invest any of their own time reviewing what should be the critical 
> code and algorithmic changes. It takes 5 minutes to unpack build and 
> start playing with the current snapshot, if you follow the instructions.
> 
> It is maybe time to *try* a slightly different approach :-).
> 
> This is a development snapshot of the parallel development tree as per 
> Raimar's request after the last update :-).
> 
> When it is time to actually code review CVS patch candidates we can talk
> about what is to go in, and do the last cycles on a consistent set of
> bite-sized pieces. For now this is code you can read and experiment with 
> to gain understanding and knowledge of the system and that should suffice.
> 
> If you want to extract pieces of it for whatever purposes, it is out
> there in the public domain for whatever benefit it provides. If you can
> look at what is here and come up with better, I'm all for it. 
> 
> Meantime, I'm having fun playing with these new features ... :-)
> 
> Cheers,
> RossW
> =====
> 
> FYI: 
> 
> 1/3 of the patch is mapgen replacement, and it would drop by 80%
> if one just sent the replacement file as opposed to the diff :-).
> 
> 1/3 (or maybe more) is reformatting changes from introducing iterate
> macros or other code updates that change (or fix) indentation. I
> maintain, the code is more understandable after reformatting especially 
> in the AI code :-).
> 
> You don't need to worry about any of the client/GUI code changes that
> basically put things back to the way they were before the recent 
> amalgamation of the GUI and core direction system broke them, or almost 
> back. 3-4 routines in each of the ai and server directory acount for most 
> of the AI stuff, with maybe a smattering of common utilities.
> 
> What you want removed is probably less than 10% of the total. And it
> is easily recognized as dealing with map addressing fixes. Most of the
> AI changes aren't map addressing related, so there is not much overlap
> to confuse you anyway.
> 
> And to put it in perspective, there have been 2-3 "patches" applied to
> CVS recently that were at least 50% of this size, or at least as
> intrusive in the numbers of files they actually touched.

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

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.

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.

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)

        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