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

[Stephan Bergmann]

On 02/01/2012 11:40 PM, julien2412 wrote:
> Cppcheck reports this :
> core/sw/source/core/fields/cellfml.cxx
> 970	StlMissingComparison	style	Missing bounds check for extra iterator
> increment in loop.
>
> 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             }
>
> Is it safe/ok ?

No, if the erased element was the only element in the sequence.

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

* "Dann mal die Tabellenkoepfe raus" means "remove the table headers."

Stephan