* Gary V. Vaughan wrote on Tue, Dec 01, 2009 at 12:02:42AM CET:
> On 30 Nov 2009, at 16:01, Ralf Wildenhues wrote:
> > * Gary V. Vaughan wrote on Mon, Nov 30, 2009 at 06:29:54PM CET:
> >> On 29 Nov 2009, at 16:27, Ralf Wildenhues wrote:
> >>> - slist_remove should IMVHO return an SList *, because otherwise there
> >>> is no way to avoid a memory leak. APIs that force memleaks are bad.
> >>
> > [[...]]
> >> Anyway, if you are proposing that SListCallback functions passed to
> >> slist_remove should always return SList *, then that does seem like a
> >> worthy addition to me.
> >
> > No. I don't propose that. I only propose it for slist_remove, because
> > that's where otherwise, a memleak is inevitable. That's just what my
> > patch does, by letting slist_remove return an SList *.
>
> We are in violent agreement! But rather than patch it the way you propose
> (which just hides the memory leak you describe from the compiler by casting
> the non-SList * valued return of SListCallback away), I suggest we enforce
> it strictly by adding a new SListRemoveCallback that ensures the callback
> function passed to slist_remove returns an SList *, and use that function
> pointer typedef for just slist_remove (in addition to changing the
> prototype of slist_remove to also return that same SList *).
But that forces slist users to write two callback functions with the
same functionality, if they also want to use it in slist_foreach for
example. And they don't gain anything in return for that.
> >> SList is designed to manage two types of lists:
> >>
> >> (i) things that were specifically designed to be chained, in which
> >> case the first field of the structures to be chained has to be
> >> 'next':
> >>
> >> struct notboxed {
> >> struct notboxed *next;
> >> <whatever real data needs to be stored in the element>
> >> };
> >>
> > [[...]]
> > I don't believe you that it was really designed to do (i). If it were,
> > then there were some code using it as such, which I would like to ask
> > you to submit as testsuite addition, so we can see whether, and *how*
> > it works at all, and the coverage may keep us from breaking it. I don't
> > believe that it works. Just like the sorting functions did not work.
>
> It really was. Early versions of slist.c did not have the boxing concept
> yet, and you had to cast your structs (with next field first) into SList *
> to use the API.
>
> It's possible (though I would be a little surprised) that the casting mode
> has been broken by the relatively recent additions of boxing...
So, there is two choices: remove the API, or add test coverage. Which
alternative do you prefer?
Thanks,
Ralf
