[Freeciv-Dev] Re: corecleanup comments

[Raimar Falke <hawk@xxxxxxxxxxxxxxxxxxxxxxx>]

On Sat, Aug 18, 2001 at 12:12:01AM -0400, Ross W. Wetmore wrote:
> 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.

If your changes are so big, maybe the changes should be done right to
the end to get this saga to an end. Convert map_adjust_* to
normalize_map_pos and than replace normalize_map_pos with
"assert(is_real_and_normalized_position(p));" (or whatever the final
will be).

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

It looks like neither of your changes will go into CVS if you will
present them in one big chunk. Everyday a patch?!

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

As said before I have some problem with the style but I will wait till
you submit something.

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

You could apply it and test it. However it is still interactive which
makes it not suited for your case.

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

        Raimar

-- 
 email: rf13@xxxxxxxxxxxxxxxxx
 "These download files are in Microsoft Word 6.0 format. After
  unzipping, these files can be viewed in any text editor, including
  all versions of Microsoft Word, WordPad, and Microsoft Word Viewer."
    -- http://www.microsoft.com/hwdev/pc99.htm