Re: Invalid iterator in RelationTableView.cxx? (dbaccess module)

On 09.02.2015 22:22, julien2412 wrote:
> Hello,
>
> On RelationTableView.cxx file, I noticed this:
>      99     for(;aIter != rTabWinDataList.rend();++aIter)
>     100     {
>     101         TTableWindowData::value_type pData = *aIter;
>     102         OTableWindow* pTabWin = createWindow(pData);
>     103 
>     104         if (!pTabWin->Init())
>     105         {
> ... 
>     112             rTabWinDataList.erase(
> ::std::remove(rTabWinDataList.begin(), rTabWinDataList.end(), *aIter),
> rTabWinDataList.end());
>     113             continue;
>     114         }
>
> idem block 139-143
>
> See
> http://opengrok.libreoffice.org/xref/core/dbaccess/source/ui/relationdesign/RelationTableView.cxx#99
>
> Shouldn't the "continue" be replaced by "break" to avoid invalid iterator or
> "aIter" isn't invalid for "for loops" at all even after "erase" lines?

good question... the aIter is a "reverse_iterator", so it starts at the
end and goes backwards...

http://www.cplusplus.com/reference/vector/vector/erase/

> Iterator validity
> Iterators, pointers and references pointing to position (or first) and beyond are invalidated,
> with all iterators, pointers and references to elements before
position (or first) are
> guaranteed to keep referring to the same elements they were referring
to before the call.

... so since the element that aIter points to will be deleted, aIter is
invalid.

if it is somehow guaranteed that there are no duplicate elements, then
the code could be fixed by incrementing aIter before doing the erase.

but wait!  a reverse_iterator actually contains an ordinary iterator
that points not to the same element, but to the *next* element following
it...

i guess that means that aIter would need to be incremented *twice* to be
safe?

hmm.... perhaps you could replace the erase(remove...) with an erase
that takes the reverse_iterator's base iterator (aIter.base()) to erase
just one element and then create a new reverse_iterator from the return
value of erase, which should be valid?  (i think you need to skip the
++aIter in this case, but better double-check that).

... why doesn't the loop use integer indexes, those are much less
confusing  :)