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@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
Attachment:
convert-viewuno-to-ptr-vector.patch
Description: application/mbox