[Freeciv-Dev] Re: improper access of iterators

[Justin Moore <justin@xxxxxxxxxxx>]

> >    I'm working on providing some profiling results.  Part of the point of
> > my post was that it's not quite as simple as re-linking against an array
> > implementation; some programmers assumed the existance of an iterator
> > named 'myiter' in a few parts of the code.  I'd have to clean out lots of
> > those places for a good comparison, resulting in some sizeable patches.
>
> How do you change these things? I would change things like
>   unit_list_iterate(pcity->units_supported, punit)
>     if (some_condition) {
>        handle_unit_disband_safe(pplayer, &pack, &myiter);
>        city_refresh(pcity);
>        ai_fix_unhappy(pcity);
>      }
>   unit_list_iterate_end;
>
> to
> for(;;) {
>   int changed=0;
>   unit_list_iterate(pcity->units_supported, punit)
>     if (some_condition) {
>        handle_unit_disband_safe(pplayer, &pack);
>        city_refresh(pcity);
>        ai_fix_unhappy(pcity);
>        changed=1;
>        break;
>      }
>   unit_list_iterate_end;
>   if(changed) break;
> }
>
> This looks save to me but is a bit ugly.

I was thinking more along the lines of:

   struct genlist_iterator myiter;
   genlist_iterator_init(&myiter);
   unit_list_iterate(pcity->units_supported, punit, pos)
     if (some_condition) {
        genlist_iterator_setpos(&myiter, pos);
        handle_unit_disband_safe(pplayer, &pack, &myiter);
        city_refresh(pcity);
        ai_fix_unhappy(pcity);
      }
   unit_list_iterate_end;

where getlist_iterator_setpos could be a macro/inline func/whatever and
pos keeps track (numerically) of which element you're looking at.
Technically it isn't in the spirit of an iterator, but from what I see in
the wipe_unit* calls, the iterator is simply there to prevent weird
dangling pointer problems.  Feel free to point out where I'm missing
something or have overlooked parts of the code I'm not familiar with.

   Code changes would be minimal and closely resemble the current code.
Another way is to change handle_unit_disband_safe (and similar functions)
to take an index into the genlist array and call genlist_unlink(genlist,
index).  My idea is to allow for gradual migration of code from old to new
in a manner similar to Ross' corecleanup changes.

> > I guess I'm just asking if it would be worth my time to do this.  I
> > feel that between the new genlist and some key sortlist
> > improvements, we could improve speed by 2-3% (IMHO), but again:
> > would the maintainers be interested in these speedups if available?
>
> IMHO it depends on the speedup and how you get rid of the myiter (how
> nice it would get).

   I'm running some profiling now to establish a good baseline.  I'll get
back to you when I've migrated some code over to the new data structures.

-jdm

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