Re: Coverage for libltdl/slist.c and fallout

[Gary V. Vaughan]

Hallo Ralf,

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

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

> If (i) does work, then likely my patch is broken after all; so I guess I
> will have to wait with applying.

I think that subject to the feedback you have received, the patch improves
slist by test coverage and bug fixing, so there's no reason to hold back
on applying in my mind.

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