Document Foundation Mail Archive › LibreOffice › Dev

Cppcheck Dangerous iterator usage after erase()-method

Classic

List

Threaded

6 messages Options

julien2412

Cppcheck Dangerous iterator usage after erase()-method

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

Markus Mohrhard

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

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
[hidden email]
http://lists.freedesktop.org/mailman/listinfo/libreoffice

julien2412

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

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

Noel Grandin-2

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

In reply to this post by Markus Mohrhard

On Sat, Sep 15, 2012 at 9:13 PM, Markus Mohrhard
<[hidden email]> 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
[hidden email]
http://lists.freedesktop.org/mailman/listinfo/libreoffice

Markus Mohrhard

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

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
[hidden email]
http://lists.freedesktop.org/mailman/listinfo/libreoffice

julien2412

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

In reply to this post by Noel Grandin-2

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

> On Sat, Sep 15, 2012 at 9:13 PM, Markus Mohrhard
> <[hidden email]> 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
[hidden email]
http://lists.freedesktop.org/mailman/listinfo/libreoffice