Mailing-List: contact dev-help@apr.apache.org; run by ezmlm
Precedence: bulk
Received-SPF: neutral (athena.apache.org: 76.96.30.96 is neither permitted nor
 denied by domain of jim@jagunet.com)
Content-Type: text/plain; charset=us-ascii
Mime-Version: 1.0 (Mac OS X Mail 7.3 \(1878.6\))
Subject: Re: Skiplist improvements
From: Jim Jagielski <jim@jaguNET.com>
In-Reply-To: 
 <CAKQ1sVONmo+GC+7dzh-xAb2b8XkFr6DzuPAC5dPv9zoecOHu_w@mail.gmail.com>
Date: Mon, 14 Jul 2014 08:23:15 -0400
Cc: apr <dev@apr.apache.org>
Content-Transfer-Encoding: 7bit
Message-Id: <169320FF-9808-4CA6-93F4-9E099B35F3FD@jaguNET.com>
References: 
 <CAKQ1sVO1EECMvSOz7Udqv83MJ_BR+KBed6wKgh8VG-ij9+qhtg@mail.gmail.com>
 <581E0F48-0128-4B06-98EC-11D49DB5E507@jaguNET.com>
 <CAKQ1sVOPhL-XCHDiFATW=uMOBxZzOrRLAC5Mvb6iuyRRwxsYzQ@mail.gmail.com>
 <4E51F5E9-FA7B-40D4-A23D-A4B4B7F56C22@jaguNET.com>
 <CAKQ1sVOwAVkFCPyy8ndgA=KBg6ANoEKE7nXZQuwDLjz+pdZz3Q@mail.gmail.com>
 <CAKQ1sVMvoOAtG6CHR2Lnhkm6qVTRfWsqQErMCtZR25K76A9J=g@mail.gmail.com>
 <CAKQ1sVM5+Pp=X_G4orYcFxtD+NxKWsut2W8yHnMmrTXrbbM6BQ@mail.gmail.com>
 <580D5FC8-CF4C-4405-9111-546FB219E085@jaguNET.com>
 <CAKQ1sVNvHQq-rbfy77aHB+rxjrarGcNxJb9YmrMkZRYYE0J43w@mail.gmail.com>
 <CAKQ1sVMNVf6CooimOmkYnW3CQk8SECT+D2Q1H+ZdfWFmM9PaLw@mail.gmail.com>
 <CAKQ1sVM1sQvLz7CYgHOxpz25eXrVSrCsSN=FOTS3W1Ti=HmsDg@mail.gmail.com>
 <CAKQ1sVONmo+GC+7dzh-xAb2b8XkFr6DzuPAC5dPv9zoecOHu_w@mail.gmail.com>
To: Yann Ylavic <ylavic.dev@gmail.com>

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