[Spice-devel] [PATCH 10/10] Change RedCharDevicePrivate::clients to GList

[Frediano Ziglio]

> 
> HHey,
> 
> On Fri, Sep 09, 2016 at 10:47:12AM -0400, Frediano Ziglio wrote:
> > > 
> > > More Ring cleanup
> > 
> > Personally I don't approve the rationale.
> > 
> > Rings are used by Qemu and Linux kernel, Qemu calls us directly and
> > we deal with Linux too. Not counting all BSD code. So surely are not
> > less tested and maintained then glib code.
> 
> In this case, 'Ring' comes from spice-common/common/ring.h, and is
> gplv2/(C) Red Hat, not sure how much of this code is common with what is
> used in kernel or BSD land
> 

Let's not investigate too much, we can risk some companies prosecute us :)
I think the original code of these stuff was public domain and it's like
looking for relatives of 10th level or more...

> 
> > Rings are not less readable or less type safe then GList or any
> > glib container (personally I think that ring_is_empty is more
> > readable than comparing a container with NULL).
> > Rings are more space and computation efficient.
> > Rings require however to embed container knowledge in the item
> > structures which not desirable in many situations.
> 
> This is my main grip with it wrt to being able to follow the code,
> random 'link' members in structures without it being clear which object
> will hold the list. Sometimes there are even several such links for
> different lists, which become even more messy. Or sometimes the link
> is just a 'base' member in the struct (is that inheritance ??). Or we
> have dummy structs just because we need one to hold the link member to
> build a list of something.
> 

About the relationship member <-> list, yes, confusing but it's
also true that now you don't know who is holding the member and how
many. Usually not an issue, unless you want to delete yourself from
the list (this for instance happens for GlzDictionary and
RedDrawablePipeItem for the Drawable list).
About the "base" name it's not a problem of ring instead of list
but the bad naming choice solvable renaming.
Yes, dummy structs are really bad but this is a specific usage case;
I could try use a linked list to implement a quick sort to demonstrate
that quick sort is slow but I think I could end up demonstrating
I'm not really a good programmer :)

> > And expanding manually macros is IMHO not more readable nor
> > more maintainable.
> 
> I don't think there is anything preventing from having a macro for
> iteration.
> 
> Christophe
> 

Yes, in this case the patches are mainly doing 3 thing:
- replacing the ring with GList (which is the written in the rationale);
- removing the cached item number;
- removing IMHO more readable macro. I think a GLIST_FOREACH(_SAFE) would
  be more readable.

Still the point is that every patch of these kind require different
attention based on the piece of code it touch, replacing containers
blindly assuming the next change is easier than the previous is a
way to introduce regressions.

Frediano