On Thu, Oct 20, 2011 at 10:50 AM, Bert Huijben <bert_at_qqmail.nl> wrote:
>
>
>> -----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?
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
>
>
Received on 2011-10-20 22:45:24 CEST