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

On 10.02.2015 10:03, Lionel Elie Mamane wrote:
> On Mon, Feb 09, 2015 at 11:00:10PM +0100, Michael Stahl wrote:
> > On 09.02.2015 22:22, julien2412 wrote:
>
> > > 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         }
>
> > > 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?
>
>
> > ... so since the element that aIter points to will be deleted, aIter
> > is invalid.
>
> I kinda assumed that an iterator pointing to the last element would
> (when that element is erased) safely point to past-the-end. I may be
> wrong.

fwiw the text i quoted says it will be invalidated.

> > but wait!  a reverse_iterator actually contains an ordinary iterator
> > that points not to the same element, but to the *next* element
> > following it...
>
> Raaah. What a headache.
>
> > i guess that means that aIter would need to be incremented *twice* to be
> > safe?
>
> Maybe, but then you break the algorithm :) It may (will?) skip the
> value before aIter.

yes.

> > 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
>
> I'm not convinced the erase is supposed to delete only one element,
> unless rTabWinDataList has no duplicate. But if it has no duplicate,

if there are duplicates then the current code is not just subtly but
obviously wrong, since on remove the aIter needs to be incremented not
just once but once for every removed duplicate.

> then maybe we can simplify all this complicated remove/erase
> construction and just do:
>
>  rTabWinDataList.erase(*aIter);
>
> No need for the "remove". Ah but then, we change the algorithm to pass

erase(aIter), that's what i meant, and the erase returns a valid
iterator and we assign that to aIter (and don't increment it so that on
the next iteration the reverse_iterator points to the previous element).

> only once on each position/value, as opposed to possibly several times
> now... Is this "several times the same value" actually desired or a
> bug? Only understanding the intent of the code will tell.

well it's probably a bug, if creating the thingy worked the first time
there's no point in trying again?