[Freeciv-Dev] (PR#11779) New genlist code

["Jason Short" <jdorje@xxxxxxxxxxxxxxxxxxxxx>]

<URL: http://bugs.freeciv.org/Ticket/Display.html?id=11779 >

- Why is sort_buffer tracked globally?  This seems entirely unnecessary
(arbitrarily wasteful of memory for no gain).  Just declare it on the
stack inside of the sort function, just like is done now.

- This comment:

+/* The below does not really belong here but has to be here
+ * for specfile to work properly. */

why does it not belong here?  Citymap iterators are in city.h; map
iterators are in map.h; why shouldn't genlist iterators be in genlist.h?

- What's the difference between genlist_new and genlist_temporary?  The
name difference means nothing to me.  Perhaps these should be
genlist_new_tracked/genlist_new_untracked or just genlist_new(tracked).

- Functions really need comments.  Especially functions like
control_init and control_done need comments to explain when they should
be called (once per program, once per connection, or once per game?).

- Do we need to add a new function for appending?  If we do, can we call
it genlist_append?  I know that speclists and specvecs have an
insert_back function, but an argument for consistency is circular since
these function names come because the speclist insert_back used to be a
wrapper for the genlist insert function.

- Functions with no parameters should be given (void) I think.  E.g.,

  void genlists_prune()

should be

  void genlists_prune(void)

this is just safer since mismatched prototypes will be more easily detected.

- genlists_prune needs more documentation.  In fact many functions in
genlist.c need better documentation.

- Casts are to be avoided IMO.  For instance this code

    if (!plink->deleted
        && plink->dataptr != NULL
        && ((struct genlist *)(plink->dataptr))->ndeleted > 0) {
      genlist_prune((struct genlist *)plink->dataptr);
    }

is unnecessarily bugprone because the casts can conceal type errors. 
The second cast is simply unnecessary.  The first cast could be avoided
with a temporary variable, although maybe this is too much work:

    struct genlist *data = plink->dataptr;

- And the only major problem: why is genlists_prune() never called at
the client?

-----

Aside from small details, the overall design seems good.

-jason