apr-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Jim Jagielski <...@jaguNET.com>
Subject Re: Skiplist improvements
Date Mon, 14 Jul 2014 12:23:15 GMT
+1
On Jul 11, 2014, at 4:31 PM, Yann Ylavic <ylavic.dev@gmail.com> wrote:

> On Fri, Jul 11, 2014 at 10:22 PM, Yann Ylavic <ylavic.dev@gmail.com> wrote:
>> There also seem to be some doubtful code in apr_skiplist_remove_all(),
>> addressed by the following patch :
>> 
>> Index: tables/apr_skiplist.c
>> ===================================================================
>> --- tables/apr_skiplist.c    (revision 1609655)
>> +++ tables/apr_skiplist.c    (working copy)
>> @@ -591,16 +591,17 @@ APR_DECLARE(void) apr_skiplist_remove_all(apr_skip
>>     m = sl->bottom;
>>     while (m) {
>>         p = m->next;
>> -        if (p && myfree && p->data)
>> -            myfree(p->data);
>> +        if (myfree && m->data)
>> +            myfree(m->data);
>>         while (m) {
>>             u = m->up;
>> -            apr_skiplist_free(sl, p);
>> +            apr_skiplist_free(sl, m);
>>             m = u;
>>         }
>>         m = p;
>>     }
>>     sl->top = sl->bottom = NULL;
>> +    sl->topend = sl->bottomend = NULL;
>>     sl->height = 0;
>>     sl->size = 0;
>> }
>> 
>> Do you agree?
> 
> Hmm, no, the initial myfree(p->data) is on purpose, bottom is a fake
> node, the real first data are on bottom->next.
> 
> Rather this patch :
> 
> Index: tables/apr_skiplist.c
> ===================================================================
> --- tables/apr_skiplist.c    (revision 1609655)
> +++ tables/apr_skiplist.c    (working copy)
> @@ -595,12 +595,13 @@ APR_DECLARE(void) apr_skiplist_remove_all(apr_skip
>             myfree(p->data);
>         while (m) {
>             u = m->up;
> -            apr_skiplist_free(sl, p);
> +            apr_skiplist_free(sl, m);
>             m = u;
>         }
>         m = p;
>     }
>     sl->top = sl->bottom = NULL;
> +    sl->topend = sl->bottomend = NULL;
>     sl->height = 0;
>     sl->size = 0;
> }
> 
> to avoid multiple free() on the same node.
> (NULLing topend and bottomend is a safety guard since they may be used
> elsewhere without top or botton being checked).


Mime
View raw message