You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Julian Foad <ju...@wandisco.com> on 2011/10/20 16:45:16 UTC
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?
- Julian
Re: svn commit: r1181090 - Follow-up to r1180154: svn_rangelist_merge2
optimization.
Posted by Julian Foad <ju...@wandisco.com>.
Paul Burba wrote:
> No, we can trim that first block. I did that along with some
> additional simplifications in r1187042.
Thanks, Paul. Looks nice now.
Sorry, Bert, for causing confusing in the email thread by not quoting
enough context.
- Julian
Re: svn commit: r1181090 - Follow-up to r1180154: svn_rangelist_merge2
optimization.
Posted by Paul Burba <pt...@gmail.com>.
On Thu, Oct 20, 2011 at 10:50 AM, Bert Huijben <be...@qqmail.nl> wrote:
>
>
>> -----Original Message-----
>> From: Bert Huijben [mailto:bert@qqmail.nl]
>> Sent: donderdag 20 oktober 2011 16:48
>> To: 'Julian Foad'; 'Paul Burba'; dev@subversion.apache.org
>> Subject: RE: svn commit: r1181090 - Follow-up to r1180154:
>> svn_rangelist_merge2 optimization.
>>
>> > -----Original Message-----
>> > From: Julian Foad [mailto:julian.foad@wandisco.com]
>> > Sent: donderdag 20 oktober 2011 16:45
>> > To: Paul Burba; dev@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?
No, we can trim that first block. I did that along with some
additional simplifications in r1187042.
>> 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.
A moot point now, but it doesn't handle it because it can't be
executed in the case where the delete_index is the last element in the
array *and* elements_to_delete > 1.
Notice the wrapping conditional around the code we are discussing here
(this context wasn't included above):
/* Do we have a valid index and are there enough elements? */
if (delete_index >= 0
&& delete_index < arr->nelts
&& elements_to_delete > 0
&& (elements_to_delete + delete_index) <= arr->nelts)
{
if (delete_index == (arr->nelts - 1))
{
/* Deleting the last or only element in an array is easy. */
apr_array_pop(arr);
}
...
}
If ((elements_to_delete + delete_index) <= arr->nelts) is true we do
nothing, exactly as the doc string promises:
/* Remove @a elements_to_delete elements starting at @a delete_index from the
* array @a arr. If @a delete_index is not a valid element of @a arr,
* @a elements_to_delete is not greater than zero, or
* @a delete_index + @a elements_to_delete is greater than @a arr->nelts,
* then do nothing.
*
* @note Private. For use by Subversion's own code only.
*/
void
svn_sort__array_delete(apr_array_header_t *arr,
int delete_index,
int elements_to_delete);
Paul
> 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
>
>
RE: svn commit: r1181090 - Follow-up to r1180154: svn_rangelist_merge2 optimization.
Posted by Bert Huijben <be...@qqmail.nl>.
> -----Original Message-----
> From: Bert Huijben [mailto:bert@qqmail.nl]
> Sent: donderdag 20 oktober 2011 16:48
> To: 'Julian Foad'; 'Paul Burba'; dev@subversion.apache.org
> Subject: RE: svn commit: r1181090 - Follow-up to r1180154:
> svn_rangelist_merge2 optimization.
>
>
>
> > -----Original Message-----
> > From: Julian Foad [mailto:julian.foad@wandisco.com]
> > Sent: donderdag 20 oktober 2011 16:45
> > To: Paul Burba; dev@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
RE: svn commit: r1181090 - Follow-up to r1180154: svn_rangelist_merge2 optimization.
Posted by Bert Huijben <be...@qqmail.nl>.
> -----Original Message-----
> From: Julian Foad [mailto:julian.foad@wandisco.com]
> Sent: donderdag 20 oktober 2011 16:45
> To: Paul Burba; dev@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.
So +1 on removing that branch.
Bert
>
> - Julian