[Freeciv-Dev] Re: [PATCH] Re: {gen,spec,sort}list stuff

[Justin Moore <justin@xxxxxxxxxxx>]

> > > > I've started to notice some inconsistencies in games using the same 
> > > > random
> > > > seeds (map and game) between games that use the old genlist and ones 
> > > > that
> > > > use my genlist.  Yet I can't find any problems in my implementation of
> > > > genlist (yes, I realize that by saying this I'm opening myself up to
> > > > someone finding incredibly obvious bugs ... which is almost the point).
> > > > I'd appreciate any extra eyes looking at the code.
> > >
> > > Is it possible to have both lists in parallel and compare their results.
> >
> >    Yes, I'll get back to everyone on this.

   I just ran a test with both lists running in parallel using bunches of
asserts around the wrapper calls to genlist_*.  There was no difference in
the contents of the two lists throughout the game, so I have no idea why
one would obtain slightly different results from the other when run
separately.  In this light, I'm submitting my genlist.[ch] files as a
patch to the current CVS tree.

> > > I'm not happy with the name REAL_.
> >
> >    SAFE_?

   For lack of comments, there are the new names.

> > >  - is this mess with the current genlist_sort and NULL needed? It is
> > >  used.
> >
> >    Could you be more specific?  I'm just replicating the external behavior
> > of genlist_sort when given a NULL compar pointer: nothing is done to the
> > list.
>
> The external behavior is stupid IMHO. It isn't used anywhere in the
> code. This can be removed in the current code. No need to code such
> no-used features in new versions.

The code in question:

 void genlist_sort(struct genlist *pgenlist,
                  int (*compar)(const void *, const void *))
 {
   if(compar == NULL)
     return;

   qsort(pgenlist->data, pgenlist->nelements, sizeof(void*), compar);
 }

   So what should I do if the compar is NULL?

>   Remove an element of the genlist with the specified user-data pointer
>   given by 'punlink'.  If there is no such element, does nothing.
>   If there are multiple such elements, removes the first one.
>
> +  idx = genlist_find_index(pgenlist, punlink);
> +  if(idx == -1)
> +    return;

   I've put an assert(idx != -1) after the function call.  This way we'll
know when something's wrong in a development version but it won't break in
a release.

   If this is accepted, I have a slew of patches (all small :)) that I can
throw out to start moving from the slower iterators to the newer ones.

-jdm

Department of Computer Science, Duke University, Durham, NC 27708-0129
Email:  justin@xxxxxxxxxxx

genlist-Aug-31-cvs.patch.gz
Description: new genlist