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

["Jason Short" <jdorje@xxxxxxxxxxxxxxxxxxxxx>]

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

> [per - Sat Jan 22 19:59:31 2005]:
> 
> Now that the API change has been done, the new genlist code itself is a
> rather small patch. Here it is.
> 
> But before I commit this, I want to make section lookups in registry.c
> into a hashtable lookup instead of a genlist search.

The design's been pretty thoroughly discussed.

Just a few minor issues with the code itself:

- What's DEBUG_LISTS?  Looks like a debugging artifact.

- When updating the copyright, I think it's wrong to remove the old
year.  "Copyright 1996" becomes "Copyright 1996-2005" or maybe
"Copyright 1996,2005".  BTW, the year is 2005 now ;-).

- For function headers that don't take any paraneters, put (void) in as
the parameter.  This is only really important for prototypes (since an
"empty" list will match any function parameters (a misfeature of C I
guess) but can save some confusion for the headers of function bodies as
well.

- What's with the LGPL bit in the header?  While it may be convenient to
have utility/ under the LGPL we cannot relicence without the approval of
all copyright holders (and I don't think you wrote this file from
scratch; at least SCO wouldn't think so).

- Tell me again why genlist_free doesn't call genlist_unlink_all? 
Having an error message or assertion here might be helpful but I can't
think of any reason why you'd want to leak memory on purpose.

- In mallocs it's better to use the variable rather than the type in the
sizeof.  E.g., instead of

  struct genlist_link *new_link
     = fc_malloc(sizeof(struct genlist_link));

use sizeof(*new_link).  Many's the time I've changed a variable type and
had to worry about updating the malloc calls as well - annoying and
avoidable.

- Avoidable casts are to be avoided.  E.g.

  && ((struct genlist *)(plink->dataptr))->ndeleted > 0) {

can use a tmp. variable and avoid a cast.

Yeah yeah, I think I mentioned some of these before.

-jason