Re: Coverage for libltdl/slist.c and fallout

[Gary V. Vaughan]

Hallo Ralf,

On 1 Dec 2009, at 06:39, Ralf Wildenhues wrote:
> * 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.

I respectfully disagree.

As a user of the SList API, IFF you know that you want to use the same callback
function in your calls to slist_remove and slist_find, then you can cast the
function pointer in your call to slist_find (or vice versa).  In the common case
where you need slist_find to return a pointer from deep inside the SList struct,
and where you also want to use slist_remove to remove an item from the list, 
then
you continue to write two callback functions, where each has its own function
pointer typedef, and where each can be typechecked at compile time.

Having the API quietly cast away some of the type checking behind the client
code's back is a worse solution IMHO.

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

I don't plan to do either.  But, of course, I'd much rather you didn't remove
half of the functionality from slist.[ch] just because I don't have time to
write unit tests.

Cheers,
    Gary
-- 
Gary V. Vaughan (address@hidden)