RE: svn commit: r1181090 - Follow-up to r1180154: svn_rangelist_merge2 optimization.

> -----Original Message-----
> From: Bert Huijben [mailto:bert_at_qqmail.nl]
> Sent: donderdag 20 oktober 2011 16:48
> To: 'Julian Foad'; 'Paul Burba'; dev_at_subversion.apache.org
> Subject: RE: svn commit: r1181090 - Follow-up to r1180154:
> svn_rangelist_merge2 optimization.
>
>
>
> > -----Original Message-----
> > From: Julian Foad [mailto:julian.foad_at_wandisco.com]
> > Sent: donderdag 20 oktober 2011 16:45
> > To: Paul Burba; dev_at_subversion.apache.org
> > Subject: Re: svn commit: r1181090 - Follow-up to r1180154:
> > svn_rangelist_merge2 optimization.
> >
> > Hi Paul.
> >
> > It looks like we can simplify this by omitting the first branch of the
> > if ... else ... else construct:
> >
> > [[[
> > if (delete_index == (arr->nelts - 1))
> > {
> > /* Deleting the last or only element in an array is easy. */
> > apr_array_pop(arr);
> > }
> > + else if ((delete_index + elements_to_delete) == arr->nelts)
> > + {
> > + /* Delete the last ELEMENTS_TO_DELETE elements. */
> > + arr->nelts -= elements_to_delete;
> > + }
> > else
> > ]]]
> >
> > Or is there some reason why 'apr_array_pop' is preferred in the N==1
> case?
>
> The first case is plain wrong. (And I should have noticed when I first reviewed
> this function)
>
> It doesn't handle elements_to_delete for N > 1.

But if the index is that last element, you can only remove one element, so it really depends on how we handle out of range values.

> So +1 on removing that branch.

This stands
>
> Bert
Received on 2011-10-20 16:50:45 CEST