[Freeciv-Dev] Re: corecleanup comments

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

At 02:18 AM 01/08/18 +0300, Gregory Berkolaiko wrote:
>actually, I have a suggestions.
>the changes you would make to packhand (and maybe pollution and few
>others :) are probably just converting explicit iterates to adjc_iterate
>and adjc_dir_iterate.  And this macros have the same call syntax in your
>patch and in current map.h.  So maybe you can submit a separate patch with
>such changes, it doesn't require all your other stuff, it will make the
>remaining patch of yours smaller => easier to handle, and it is very
>useful (even if your core patch doesn't get it through which I hope it
>will).

I wish ... I *have* split the diff (hand teeing the full diff -ru into the
various subsets is getting to be automatic :-), but only about 27K of the
327K is non-controversial and use just the bare iterates or flip the
map_adjust_* -> normalize_map_pos. When I have gone through the systemmatic
patch, rebuild and test cycle, I will at least put these out as a formal
patch request for CVS, along with the autogame subset. Gaute has found a
number of these though, and may be ready to put his collection into CVS.
Either way they will get in in one form or another before long, so they are
not a real problem.

With a little cleverness, I can probably do some minimal map.h updates and
get most of the goto.c, gotohand.c and settlers.c stuff. But the code here
is a prime candidate for an initial 'indent -kr2' and is not trivial to
review by inspection. Plus Jason and I are going to step all over ourselves
here until one of us gets into CVS.

The next big section rev's things to DX2 order, i.e. NW or 0,0 coordinate
origin, and probably takes in a big hunk of map.c/map.h plus city.c/city.h
which people are almost ready for. I don't really want to split it from the
full WRAP changes as there will be a enough intermediate code hacking to
get something that is moderately stable and the full patch has run 48 hours
of continuous autogaming so I am reasonably comfortable with it - at least
the server parts.

Anyway, I will be running most of the patch as a parallel branch for a
couple more updates against current CVS snapshots, so those that want to
help locate the residual bad accesses can drop a snapshot, patch and build
a fresh system in short order.

But if you actually want to play as opposed to find bad code, then compile
with NDEBUG to kill the asserts and fix the divide by "0" trap in this
section and you should run more or less like the current CVS.

/* Note:
 * Unlike map_normalize, map_adjust faults when it cannot return a valid
 * adjusted coordinate, i.e. out-of-bounds with no wrap in that dimension.
 * One gets a safer version of this by using the full normalize_map_pos()
 * and checking the return status.
 * For final release, this behaviour can be relaxed by removing the "/0".
 */
#define map_adjust(WRAPTYPE,BASE,POS)
  \
( (unsigned)(POS) < (unsigned)(BASE) ? (POS)             /* normal map pos
*/ \
  : ( !has_mapwrap(WRAPTYPE) ? (POS)/0                /* unfixable->assert
*/ \
      : ( (POS)%(BASE) < 0 ? (POS)%(BASE)+(BASE)        /* normalize x pos
*/ \
          : (POS)%(BASE)) ) )

#define map_adjust_x(X)         map_adjust(MAP_TYPE_WRAPX,map.xsize,(X))
#define map_adjust_y(Y)         map_adjust(MAP_TYPE_WRAPY,map.ysize,(Y))

   
>> If you have any thoughts on how to automate testing some of the GUI code
>> that might really help. Part of the problem is, if I leave the client
>> running in an autogame the network buffers overflow during big updates and
>> it crashes.
>
>I tried running autogame and nothing crashes.  that's probably because I
>don't know anything about network buffers ;)

You mean you connected a client as a valid CIV player, toggled to AI and
ran with timeout set to "1"? For me I don't get more than a few turns of
the explorer racing around the map before the system mis-communicates. And
even with this, I am not sure how much of the client codebase is actually
touched. I would like to use Raimar's client AI to at least simulate a
human user but the final version of that is still a ways off, and I'm not
currently doing much to help him out :-).

BTW: once there is a reasonable client and server auto-test it should be
trivial to add it as a final step to the master make and have the nightly
builds run this as a sanity check against the days updates.

>G.

Cheers,
RossW