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

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

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.

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

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

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

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.

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

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.

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

>       Raimar
>
>-- 
> email: rf13@xxxxxxxxxxxxxxxxx
>  A supercomputer is a computer running an endless loop in just a second

Cheers,
RossW
======