Invalid iterator in RelationTableView.cxx? (dbaccess module)

[Lionel Elie Mamane]

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.

> 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 then, you have to avoid the for loop to increment it again :)

My first though was something like:

for(;aIter != rTabWinDataList.rend();)
{
  ...
  if (!pTabWin->Init())
  {
    ...
    rTabWinDataList.erase(::std::remove(rTabWinDataList.begin(), rTabWinDataList.end(), *aIter++),
                          rTabWinDataList.end());
    ...
    continue;
  }
  else
    ++aIter;
  ...
}

but actually we could do

for(;aIter != rTabWinDataList.rend();)
{
  TTableWindowData::value_type pData = *aIter++;
  OTableWindow* pTabWin = createWindow(pData);

  if (!pTabWin->Init())
  {
    ...
    auto aOldIter = aIter;
    rTabWinDataList.erase(::std::remove(rTabWinDataList.begin(), rTabWinDataList.end(), pData),
                          rTabWinDataList.end());
    ...
    continue;
  }
  ...
}

> 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.

> 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,
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
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.

-- 
Lionel