[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 Sun, 9 Jan 2005, Jason Short wrote:

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

new is a good name (most people are used to it). I do not know if making
this an argument is a good idea however. It is bug prone, most people will
not read the API header and just copy & paste an example even if it is
documented. At least I know I don't read documentation, if I can help it! :-)

I had another idea. Why not just make the tracker list track itself? As
long as the tracker list is the first list in the tracker list, it should
work fine and the code will be more elegant!

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

Yes. I would even rename them both to append. It is both shorter and more
obvious. Although for genlists, I think the old generic insert function
was a better idea. Positive index numbers count from the beggining of the
list while negative index numbers count from the end of the list. Just
like in Python.

Specvecs do not have an insert function, because such a function is
complex and computationally expensive.

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

Yes. This isn't C++.

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

You know how I feel about 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.

Yes, it makes much more sense than the other things we tried before. It is
sort of like list garbage collection, but you can call the garbage
collector on a list whenever you want.

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