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

["Vasco Alexandre da Silva Costa" <vasc@xxxxxxxxxxxxxx>]

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

On Sat, 29 Jan 2005, Jason Short wrote:

> <URL: http://bugs.freeciv.org/Ticket/Display.html?id=11779 >
>
> - 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 ;-).

'Copyright 1996-2005' or 'Copyright 1996..2005'. The comma doesn't make it
obvious that there was intermediate work.

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

Yes, this isn't C++. Into the 'void' we must go. :-)

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

Agreed. I keep forgetting to do it. Adding this would make code less
memory leak prone and have less tedious typing involved.

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

Indeed. I always use this form because of that. It also makes more sense
really.

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

Yes, I also dislike sticking casts in the middle of expressions, most of
the time.

---
Vasco Alexandre da Silva Costa @ Instituto Superior Tecnico, Lisboa