[Freeciv-Dev] Re: [PATCH] Corecleanup_06 has been put in incoming

[Raimar Falke <hawk@xxxxxxxxxxxxxxxxxxxxxxx>]

On Wed, Aug 22, 2001 at 08:07:25PM -0400, Ross W. Wetmore wrote:
> Just to answer some of the questions, I've been liberal with trimming.
> 
> At 06:04 PM 01/08/22 +0200, Raimar Falke wrote:
> >On Sun, Aug 19, 2001 at 01:54:20PM -0400, Ross W. Wetmore wrote:
> >> I have dropped a consistent set of patches to the Aug-15 CVS snapshot into
> >> the incoming directory along with a ReadMe attached below.
> [...]
> >> diff -ru --exclude-from .DiffX .freeciv-Aug-15JS/ai/advdomestic.c
> freeciv/ai/advdomestic.c
> >
> >You know that there is a diff_ignore file?!
> 
> I saw it after I had pretty much duplicated it genning my own :-(.
> 
> [...]
> >> -    if (is_worker_here(pcity, x, y)) {
> >> -      if (map_get_special(pcity->x+x-2, pcity->y+y-2) & S_ROAD) i++;
> >> +    if (map_get_special(x, y) & S_ROAD) {
> >> +      if (is_worker_here(pcity, cx,cy)) i++;
> >
> >Since you already changed the order: what about a "map_get_special()
> >&& is_worker_here()"?
> 
> The surrounding code blocks followed a pattern for which the noise fix
> corrected a flaw. There are lines like the kludge i++ that run inside 
> the first if block in some cases. Thus it doesn't seem reasonable to
> condense the two tests as the second one is really one of a set of
> possible things that can happen if the first of the tests is true.

Ok.

> [...]
> >> -  if (!pcity) return 0;
> >> +  if (pcity) {
> >
> >I prefer the old way. Bail out as early as you can. This saves some
> >intention space.
> 
> My experience is that having multiple returns from a routine makes it
> more difficult to understand what it is doing in many cases where the
> code blocks are short like the above. 
> 
> Also, the if(<condition>) { <if-clause> } is very clear that the condition
> guards the internal clause. A return can be for many different reasons
> and isn't as obviously associated with the protected code.
> 
> These aren't rigorous things, just minor subtleties that are part of
> 30+ years of accumulated style and experience. 
> 
> You should read "Writing Solid Code" by Steve Maguire as an interesting
> collection of a lot of minor stylish things that can do a lot to make
> code more robust and easier to grasp.
> 
> The "if ( <const> == <variable> )" trick which allows the compiler to
> catch typos like "if( x = 0 )" that are very difficult to spot otherwise 
> is one of the things he talks about.

Ok we have both declared your positions. It looks like there will be
no consensus and I won't want to start another thread.

> >> +    if( ptile && ptile != &void_tile ) {
> >
> >Why this extra check for void_tile? IMHO this shouldn't needed.
> 
> The check for void_tile is required because map_get_tile returns
> void_tile as the (disgusting, bad, evil, ...) hack to catch off map 
> accesses. I changed this in the map.h macro flavour in corecleanup_06b
> to return a NULL to trap a lot of the occurences and eliminate them.
> This line should go away at some point, and if it is where I think
> it is it may have been made redundant by an iterate macro. But there
> were a couple spots which I treated as controversial and left in the 
> later patch sets, adding the NULL check let it pass either the original
> code or my NULL trap until this section was resolved.
> 
> To summarize, during intermediate cleanup patches, either NULL or 
> void_tile may (correctly) signal a bad tile here. In the utopian future
> neither will be required.

Can you please spare place where things like happen from the immediate
cleanup patches?

> **************************************************************************/
> >>  static void find_city_beach( struct city *pc, struct unit *punit, int
> *x, int *y)
> >>  {
> >> -  int i, j;
> >> -  int xx, yy, best_xx = punit->x, best_yy = punit->y;
> >> -  int dist = 100;
> >> +  int best_xx = punit->x, best_yy = punit->y;
> >> +  int dist = 100;  /* should this be fn(map.xsize+map.ysize) ??? */
> >
> >I usually do something like this:
> >
> > int best=-1;
> > for(something)
> > { 
> >    int current=f();
> >    if(current>best || best==-1)
> >    {
> >       best=current;
> >    }
> > }
> 
> This change just eliminates the unneeded x,y and i,j variables. I don't
> think I touched the underlying code in any purely cosmetic way, i.e.
> unless I was doing something to it.
> 
> I won't go into the bracketting style discussion ...

Ahh so you now know my real personal style.

> BTW: if anyone has any feedback on whether the dist = 100 should be
> changed to vary as some function of the maximum map distance, this might
> be a good thing to put in. I leave comments like this in passing both
> for code review and for long term maintenance if the answers aren't 
> immediately available.

If you replace the handling in find_city_beach with the construct
above I will accept a seperate patch. No need to worry about if dist
is big enough or not.

> >This is always save if there is an unused value (the -1 here) available.
> >
> >> -  /* Catches attempts to move off map */
> >> -  if (!is_real_tile(dest_x, dest_y))
> >> -    return;
> >> -  send_move_unit(&req_unit);
> >> +  /* Catches attempts to move off map and normalizes wrapped coords */
> >> +  if (normalize_map_pos(&dest_x, &dest_y)) {
> >> +    req_unit = *punit;
> >> +    req_unit.x = dest_x;
> >> +    req_unit.y = dest_y;
> >> +    send_move_unit(&req_unit);
> >> +  }
> >
> >I would here also prefer the old form.
> 
> See above :-)
> 
> >> +    int y= pcity->y - r;
> >> +    normalize_map_pos(&x, &y);
> >
> >An extra assert()?
> >
> >> +    update_map_canvas( x, y, d, d, 1);
> 
> I looked at this and came to the conclusion that it was just possible
> that you should send any coordinates through, as update_map_canvas does
> the right thing, and I think you may want it to paint black tiles. The
> fix corrects the basic normalization problem, i.e. the only necessary 
> update.
> 
> I would not add the assert in case it breaks real behaviour, but I guess
> one could try and see if the game still runs when you scroll the client
> off into the black. I'd have a GUI person look at this, and do it with
> a full set of core_cleanup patches in a flat-earth world scrolling off 
> the x-edge to be absolutely sure.
> 
> >You were really hard-working. Besides this things above I like the
> >patch. However it is just too big. Can you please make several
> >patches: one replacing square_iterate, one replacing adjc_iterate, one
> >whole_map_iterate, one city_map_iterate_checked and one with the rest
> >and the cases above?
> 
> What is the purpose of smaller patches, really? 
> 
> Why break the iterate ones into separate pieces? while leaving the rest
> as a lump?

Because my attention won't last the 20k lines of your patch.

> This just looks like unnecessary work, but if you can convince me there 
> is a valid technical reason, I'll spend the hours teeing the diffs and
> rerunning the build/test cycles to make sure nothing got missed.

Either this or I will you rip you in serveral rounds the pieces which
are obvious (>80%) from your patch.

> >It is really amazing how many cruft is in there.
> 
> I'm happy somone has actually looked through it and can appreciate the
> bigger picture rather dealing with it as an endless stream of minor 
> bugfixes and tweaks.
> 
> Really, once this sort of stuff is standardized throughout the Freeciv
> codebase you really can do multiple map topologies with a relatively
> minor set of changes.

Yes.

        Raimar

-- 
 email: rf13@xxxxxxxxxxxxxxxxx
    1) Customers cause problems.
    2) Marketing is trying to create more customers.
  Therefore:
    3) Marketing is evil.