[Libreoffice] Question about iterator management in sw/source/core/fields/cellfml.cxx

[Lubos Lunak]

On Thursday 02 of February 2012, Stephan Bergmann wrote:
> On 02/01/2012 11:40 PM, julien2412 wrote:
> > Here are the lines :
> >      961             // dann mal die Tabellenkoepfe raus:
> >      962             for( SwSelBoxes::iterator it = rBoxes.begin(); it !=
> > rBoxes.end(); ++it )
> >      963             {
> >      964                 pLine = it->second->GetUpper();
> >      965                 while( pLine->GetUpper() )
> >      966                     pLine = pLine->GetUpper()->GetUpper();
> >      967
> >      968                 if( pTbl->IsHeadline( *pLine ) )
> >      969                 {
> >      970                     rBoxes.erase( it++ );
> >      971                     --it;
> >      972                 }
> >      973             }

> > patch proposed
> > http://nabble.documentfoundation.org/file/n3708331/sw_patch.txt
> > sw_patch.txt
>
> The patch solves that, but I can offer a number of remarks:
>
> * I would not replace the for loop with a while one, it unnecessarily
> makes the code longer, with the general additional minor drawback of
> enlarging the scope of the index variable.  I also would not extract
> aEnd; it makes the code longer and requires a tiny additional mental
> step to ascertain oneself that it is indeed not invalidated by the loop
> body.
>
> * You accidentally changed the indentation of the loop body.
>
> * Hungarian notation is ugly, esp. for trivialities like loop index
> variables.

 I agree with all the points, but in Julien's defense, I remember exactly this 
same approach was pushed in recently as a fix to the same issue elsewhere. 
Perhaps we should agree on what the recommended way is? I personally think 
the simplest and most elegant solution is to go with 'it = container.erase( 
it );" and move the "++it" out of for()'s parentheses to an else block of the 
if() condition (oh well, ease of use clearly wasn't one of priorities for STL 
designers).

-- 
 Lubos Lunak
 l.lunak at suse.cz