[svn.haxx.se] · SVN Dev · SVN Users · SVN Org · TSVN Dev · TSVN Users · Subclipse Dev · Subclipse Users · this month's index

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

From: Paul Burba <ptburba_at_gmail.com>
Date: Thu, 20 Oct 2011 16:44:53 -0400

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

This is an archived mail posted to the Subversion Dev mailing list.