[PATCH] convert SV_DECL_PTRARR_DEL to boost::ptr_vector in sc/inc/viewuno.hxx

[Noel Grandin]

Hi

New patch with recommended fixes attached.

I wonder, would it not be better to convert this type of listener-list 
thing to boost::ptr_set?
- no chance of accidentally adding duplicates
- O(1) deletion

Regards, Noel Grandin

On 2012-02-24 18:33, Ivan Timofeev wrote:
> Hi Noel,
>
>> From: Noel Grandin <noel at ubuntu.(none)>
> Do you really want to be @ubuntu.(none)? If not, use
>  $ git config --global user.email <your_address_here>
>
>> -        for ( sal_uInt16 n=0; n<aActivationListeners.Count(); n++ )
>> +        for (XActivationEventListenerVector::iterator it = 
>> aActivationListeners.begin(); it != aActivationListeners.end(); ++it)
>>          {
>>              try
>>              {
>> -                
>> (*aActivationListeners[n])->activeSpreadsheetChanged( aEvent );
>> +                (*it)->activeSpreadsheetChanged( aEvent );
>>              }
>>              catch( uno::Exception& )
>>              {
>> -                aActivationListeners.DeleteAndDestroy( n );
>> -                --n; // because it will be increased again in the loop
>> +                it = aActivationListeners.erase( it);
>>              }
>>          }
>
> 'erase' returns an iterator to the *following* element, and that 
> element will be skipped by "++it"; we must not increment the iterator 
> after erase.
> I don't know what is the most common idiom for this case, my proposal is
>
> -        for ( sal_uInt16 n=0; n<aActivationListeners.Count(); n++ )
> +        for (XActivationEventListenerVector::iterator it = 
> aActivationListeners.begin(); it != aActivationListeners.end(); )
>          {
>              try
>              {
> -                (*aActivationListeners[n])->activeSpreadsheetChanged( 
> aEvent );
> +                (*it)->activeSpreadsheetChanged( aEvent );
> +                ++it;
>              }
>              catch( uno::Exception& )
>              {
> -                aActivationListeners.DeleteAndDestroy( n );
> -                --n; // because it will be increased again in the loop
> +                it = aActivationListeners.erase( it);
>              }
>          }
>
>
> Similarly, in the following example
>
>> -    sal_uInt16 nCount = aMouseClickHandlers.Count();
>> -    for ( sal_uInt16 n=nCount; n--; )
>> +    sal_uInt16 nCount = aMouseClickHandlers.size();
>> +    for (XMouseClickHandlerVector::iterator it = 
>> aMouseClickHandlers.begin(); it != aMouseClickHandlers.end(); ++it)
>>      {
>> -        uno::Reference<awt::XEnhancedMouseClickHandler> *pObj = 
>> aMouseClickHandlers[n];
>> +        uno::Reference<awt::XEnhancedMouseClickHandler> *pObj = &(*it);
>>          if ( *pObj == aListener )
>> -            aMouseClickHandlers.DeleteAndDestroy( n );
>> +            it = aMouseClickHandlers.erase(it);
>>      }
>
> should be
>
> -    sal_uInt16 nCount = aMouseClickHandlers.Count();
> -    for ( sal_uInt16 n=nCount; n--; )
> +    sal_uInt16 nCount = aMouseClickHandlers.size();
> +    for (XMouseClickHandlerVector::iterator it = 
> aMouseClickHandlers.begin(); it != aMouseClickHandlers.end();)
>      {
> -        uno::Reference<awt::XEnhancedMouseClickHandler> *pObj = 
> aMouseClickHandlers[n];
> +        uno::Reference<awt::XEnhancedMouseClickHandler> *pObj = &(*it);
>          if ( *pObj == aListener )
> -            aMouseClickHandlers.DeleteAndDestroy( n );
> +            it = aMouseClickHandlers.erase(it);
> +        else
> +            ++it;
>      }
>
> or maybe it would be better without pObj:
>
> -    sal_uInt16 nCount = aMouseClickHandlers.Count();
> -    for ( sal_uInt16 n=nCount; n--; )
> +    sal_uInt16 nCount = aMouseClickHandlers.size();
> +    for (XMouseClickHandlerVector::iterator it = 
> aMouseClickHandlers.begin(); it != aMouseClickHandlers.end();)
>      {
> -        uno::Reference<awt::XEnhancedMouseClickHandler> *pObj = 
> aMouseClickHandlers[n];
> -        if ( *pObj == aListener )
> -            aMouseClickHandlers.DeleteAndDestroy( n );
> +        if ( *it == aListener )
> +            it = aMouseClickHandlers.erase(it);
> +        else
> +            ++it;
>      }
>
> Also, I *think* changing the loop order (descending->ascending) is 
> safe here, so
>
>> -    sal_uInt16 nCount = aPropertyChgListeners.Count();
>> -    for ( sal_uInt16 n=nCount; n--; )
>> +    for (size_t i = aPropertyChgListeners.size(); i--; )
>
> may be converted to a iterator-based loop as well. Or is that intended?
>
> Regards,
> Ivan
>

Disclaimer: http://www.peralex.com/disclaimer.html


-------------- next part --------------
A non-text attachment was scrubbed...
Name: convert-viewuno-to-ptr-vector.patch
Type: application/mbox
Size: 23685 bytes
Desc: not available
URL: <http://lists.freedesktop.org/archives/libreoffice/attachments/20120229/dcd059a1/attachment-0001.bin>