Re: Cppcheck Dangerous iterator usage after erase()-method

2012-09-16 Thread Julien Nabet 

On 16/09/2012 12:26, Noel Grandin wrote:

On Sat, Sep 15, 2012 at 9:13 PM, Markus Mohrhard
  wrote:

Hey,



Here are the lines:
 178 void SwBlink::FrmDelete( const SwRootFrm* pRoot )
 179 {
 180 for( SwBlinkList::iterator it = aList.begin(); it !=
aList.end(); )
 181 {
 182 if( pRoot == (*it).GetRootFrm() )
 183 aList.erase( it );
 184 else
 185 ++it;
 186 }
 187 }

I must recognize, I don't understand how can it work above all if we go  in
the "if" since there's no increment.

Any idea?

It can't. Line 183 is supposed to be:

aList.erase(it++);


No, it should be
it = aList.erase(it);

Once you have called erase(), the iterator becomes invalid, so it must
be replaced by the iterator returned by erase(), which returns the
next valid position,
Except that SwBlinkList inherits from boost::ptr_set, I don't know how 
iterator works with this class.
Some container returns an iterator when you call erase method, some not. 
Sometimes it depends on if you use C++11
Moreover what kind of things can invalidate iterator still seem hard to 
remember (see 
http://cb.nowan.net/blog/2004/12/30/what-will-invalidate-your-iterators/).

So I let experts decide :)

Julien
___
LibreOffice mailing list
LibreOffice@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/libreoffice

Re: Cppcheck Dangerous iterator usage after erase()-method

2012-09-16 Thread Markus Mohrhard 
Hey Noel,

>>
>> It can't. Line 183 is supposed to be:
>>
>> aList.erase(it++);
>>
>
> No, it should be
>it = aList.erase(it);
>
> Once you have called erase(), the iterator becomes invalid, so it must
> be replaced by the iterator returned by erase(), which returns the
> next valid position,

In a set my version is totally correct. it++ returns the old iterator
and then increments it, so we increment before we erase the element
and in sets only iterators to the erased element are invalidated.

For lists and sets the code I have shown is a common idiom to delete an element.

Regards,
Markus
___
LibreOffice mailing list
LibreOffice@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/libreoffice

Re: Cppcheck Dangerous iterator usage after erase()-method

2012-09-16 Thread Noel Grandin 
On Sat, Sep 15, 2012 at 9:13 PM, Markus Mohrhard
 wrote:
> Hey,
>
>
>>
>> Here are the lines:
>> 178 void SwBlink::FrmDelete( const SwRootFrm* pRoot )
>> 179 {
>> 180 for( SwBlinkList::iterator it = aList.begin(); it !=
>> aList.end(); )
>> 181 {
>> 182 if( pRoot == (*it).GetRootFrm() )
>> 183 aList.erase( it );
>> 184 else
>> 185 ++it;
>> 186 }
>> 187 }
>>
>> I must recognize, I don't understand how can it work above all if we go  in
>> the "if" since there's no increment.
>>
>> Any idea?
>
> It can't. Line 183 is supposed to be:
>
> aList.erase(it++);
>

No, it should be
   it = aList.erase(it);

Once you have called erase(), the iterator becomes invalid, so it must
be replaced by the iterator returned by erase(), which returns the
next valid position,
___
LibreOffice mailing list
LibreOffice@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/libreoffice

Re: Cppcheck Dangerous iterator usage after erase()-method

2012-09-15 Thread julien2412 
Thank you Markus. I commited and pushed on master. (see
https://gerrit.libreoffice.org/gitweb?p=core.git;a=commitdiff;h=0d40167cec11faee1488397fa323ae4511ac2050)

It seems to be only on master so no need to cherry-pick for other branches.

Julien



--
View this message in context: 
http://nabble.documentfoundation.org/Cppcheck-Dangerous-iterator-usage-after-erase-method-tp4007715p4007724.html
Sent from the Dev mailing list archive at Nabble.com.
___
LibreOffice mailing list
LibreOffice@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/libreoffice

Re: Cppcheck Dangerous iterator usage after erase()-method

2012-09-15 Thread Markus Mohrhard 
Hey,


>
> Here are the lines:
> 178 void SwBlink::FrmDelete( const SwRootFrm* pRoot )
> 179 {
> 180 for( SwBlinkList::iterator it = aList.begin(); it !=
> aList.end(); )
> 181 {
> 182 if( pRoot == (*it).GetRootFrm() )
> 183 aList.erase( it );
> 184 else
> 185 ++it;
> 186 }
> 187 }
>
> I must recognize, I don't understand how can it work above all if we go  in
> the "if" since there's no increment.
>
> Any idea?

It can't. Line 183 is supposed to be:

aList.erase(it++);

Regards,
Markus
___
LibreOffice mailing list
LibreOffice@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/libreoffice

Cppcheck Dangerous iterator usage after erase()-method

2012-09-15 Thread julien2412 
Hello,

Cppcheck reported this:
[sw/source/core/text/blink.cxx:183]: (error) Dangerous iterator usage after
erase()-method.

Here are the lines:
178 void SwBlink::FrmDelete( const SwRootFrm* pRoot )
179 {
180 for( SwBlinkList::iterator it = aList.begin(); it !=
aList.end(); )
181 {
182 if( pRoot == (*it).GetRootFrm() )
183 aList.erase( it );
184 else
185 ++it;
186 }
187 }

I must recognize, I don't understand how can it work above all if we go  in
the "if" since there's no increment.

Any idea?

Julien



--
View this message in context: 
http://nabble.documentfoundation.org/Cppcheck-Dangerous-iterator-usage-after-erase-method-tp4007715.html
Sent from the Dev mailing list archive at Nabble.com.
___
LibreOffice mailing list
LibreOffice@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/libreoffice