Re: Question about iterator management in sw/source/core/fields/cellfml.cxx

2012-02-03 Thread Stephan Bergmann 

On 02/02/2012 09:08 PM, julien2412 wrote:

Would this patch better ? (I kept the for loop)


Unfortunately that still has a problem.  After rBoxes.erase(toErase), 
it (which is the same as toErase) is invalidated, so incrementing it 
(up in the for(...;...;...) part) has undefined behavior.


The standard idiom is

  for (iterator i = m.begin(); i != m.end();) {
if (doErase) {
  m.erase(i++);
} else {
  ++i;
}
  }


diff --git a/sw/source/core/fields/cellfml.cxx
b/sw/source/core/fields/cellfml.cxx
index 33b953e..5c626dd 100644
--- a/sw/source/core/fields/cellfml.cxx
+++ b/sw/source/core/fields/cellfml.cxx
@@ -967,8 +967,8 @@ void SwTableFormula::GetBoxes( const SwTableBox
rSttBox,

  if( pTbl-IsHeadline( *pLine ) )
  {
-rBoxes.erase( it++ );
---it;
+SwSelBoxes::iterator toErase = it;
+rBoxes.erase( toErase );
  }
  }
  } while( sal_False );

I read that to erase a position on a iterator invalidate this iterator from
position to the end
(http://www.cplusplus.com/reference/stl/vector/erase/)
I don't know if to create this temporary variable toErase change something
or it's all the same.


The text you cite is about vector, for which *all* iterators into a 
vector are invalidated upon an erase.  For map (which SwSelBoxes is), 
only iterators pointing to the erased element are invalidated.



(sorry for the nitpicking but I'd like to understand this point)


No problem at all, and I hope my nitpicking helps shed some light.

Stephan
___
LibreOffice mailing list
LibreOffice@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/libreoffice

Re: Question about iterator management in sw/source/core/fields/cellfml.cxx

2012-02-03 Thread Michael Stahl 
On 03/02/12 14:01, Stephan Bergmann wrote:
 On 02/02/2012 09:08 PM, julien2412 wrote:
 Would this patch better ? (I kept the for loop)
 
 Unfortunately that still has a problem.  After rBoxes.erase(toErase), 
 it (which is the same as toErase) is invalidated, so incrementing it 
 (up in the for(...;...;...) part) has undefined behavior.
 
 The standard idiom is
 
for (iterator i = m.begin(); i != m.end();) {
  if (doErase) {
m.erase(i++);
  } else {
++i;
  }
}

but doesn't that have the same problem? i is incremented only after
the erase is complete, when i is already invalid.

shouldn't this be something like:

for (iterator i = m.begin(); i != m.end(); ) {
if (doErase) {
   iterator const j = i++;
   m.erase(j);
} else {
   ++i;
}
}

 I read that to erase a position on a iterator invalidate this iterator from
 position to the end
 (http://www.cplusplus.com/reference/stl/vector/erase/)
 I don't know if to create this temporary variable toErase change something
 or it's all the same.
 
 The text you cite is about vector, for which *all* iterators into a 
 vector are invalidated upon an erase.

i think i read this invalidate from position to end in the SGI
documentation as well; does the actual standard give implementations
more freedom here?


___
LibreOffice mailing list
LibreOffice@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/libreoffice

Re: Question about iterator management in sw/source/core/fields/cellfml.cxx

2012-02-03 Thread Stephan Bergmann 

On 02/03/2012 04:18 PM, Michael Stahl wrote:

On 03/02/12 14:01, Stephan Bergmann wrote:

The standard idiom is

for (iterator i = m.begin(); i != m.end();) {
  if (doErase) {
m.erase(i++);
  } else {
++i;
  }
}


but doesn't that have the same problem? i is incremented only after
the erase is complete, when i is already invalid.


No, i is incremented before calling erase in the above code.


The text you cite is about vector, for which *all* iterators into a
vector are invalidated upon an erase.


i think i read this invalidate from position to end in the SGI
documentation as well; does the actual standard give implementations
more freedom here?


Oh yes, vector erase only invalidates all the iterators after the point 
of erase, not each and every iterator into the vector (i.e., it may not 
reallocate the shrunk array to a smaller memory area).  Sorry for the 
confusion.


Stephan
___
LibreOffice mailing list
LibreOffice@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/libreoffice

Re: Question about iterator management in sw/source/core/fields/cellfml.cxx

2012-02-03 Thread Michael Stahl 
On 03/02/12 17:21, Stephan Bergmann wrote:
 On 02/03/2012 04:18 PM, Michael Stahl wrote:
 On 03/02/12 14:01, Stephan Bergmann wrote:
 The standard idiom is

 for (iterator i = m.begin(); i != m.end();) {
   if (doErase) {
 m.erase(i++);
   } else {
 ++i;
   }
 }

 but doesn't that have the same problem? i is incremented only after
 the erase is complete, when i is already invalid.
 
 No, i is incremented before calling erase in the above code.

ah, that's surprising.
see, that is why i almost always write the i++ as an extra statement,
i'm never quite exactly sure what it does, and when  :)

___
LibreOffice mailing list
LibreOffice@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/libreoffice

Re: Question about iterator management in sw/source/core/fields/cellfml.cxx

2012-02-03 Thread Stephan Bergmann 

On 02/03/2012 05:31 PM, Michael Stahl wrote:

ah, that's surprising.


that's shocking ;)


see, that is why i almost always write the i++ as an extra statement,
i'm never quite exactly sure what it does, and when  :)


There is a sequence point (in C++03 parlance; the nomenclature changed 
slightly for C++11) after evaluation of all function arguments, before 
any statements of the function body are executed.  Therefore, erase(i++) 
fully executes i++ before calling erase (with the old value of i).


Stephan
___
LibreOffice mailing list
LibreOffice@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/libreoffice

Re: Question about iterator management in sw/source/core/fields/cellfml.cxx

2012-02-03 Thread Terrence Enger 
On Fri, 2012-02-03 at 17:31 +0100, Michael Stahl wrote:
 On 03/02/12 17:21, Stephan Bergmann wrote:
  On 02/03/2012 04:18 PM, Michael Stahl wrote:
  On 03/02/12 14:01, Stephan Bergmann wrote:
  The standard idiom is
 
  for (iterator i = m.begin(); i != m.end();) {
if (doErase) {
  m.erase(i++);
} else {
  ++i;
}
  }
 
  but doesn't that have the same problem? i is incremented only after
  the erase is complete, when i is already invalid.
  
  No, i is incremented before calling erase in the above code.
 
 ah, that's surprising.
 see, that is why i almost always write the i++ as an extra statement,
 i'm never quite exactly sure what it does, and when  :)

Stephan, I am sorry to question your expertise, but I wonder ... is
your reassurance based on knowledge of the language standard, or is
based on observed behaviour of C++ compilers?

It is guaranteed that i will be incremented after it is copied (I am
*assuming* call-by-value.) for use by erase().  I suspect (but do not
know) that the question of whether the increment happens before or
after the call to erase() may be implementation-defined or worse.

(This, of course, is independent of what STL says about the validity
of iterators after erase().)

Terry, who says
This is your last warning.  I have a semicolon, and I am not
afraid to use it.


___
LibreOffice mailing list
LibreOffice@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/libreoffice

Re: Question about iterator management in sw/source/core/fields/cellfml.cxx

2012-02-03 Thread Michael Stahl 
On 03/02/12 18:17, Terrence Enger wrote:

 Stephan, I am sorry to question your expertise, but I wonder ... is
 your reassurance based on knowledge of the language standard, or is
 based on observed behaviour of C++ compilers?

hahaha, i believe that among all the developers who ever worked on
OpenOffice.org code though its long history, Stephan is the only one who
actually _understands_ C++  :)

___
LibreOffice mailing list
LibreOffice@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/libreoffice

Re: Question about iterator management in sw/source/core/fields/cellfml.cxx

2012-02-03 Thread Terrence Enger 
On Fri, 2012-02-03 at 18:30 +0100, Michael Stahl wrote:
 On 03/02/12 18:17, Terrence Enger wrote:
 
  Stephan, I am sorry to question your expertise, but I wonder ... is
  your reassurance based on knowledge of the language standard, or is
  based on observed behaviour of C++ compilers?
 
 hahaha, i believe that among all the developers who ever worked on
 OpenOffice.org code though its long history, Stephan is the only one who
 actually _understands_ C++  :)

Ah, yes.

In my question, I left out the words sequence point for fear of 
suggesting a level of understanding beyond what I actually have.

Thank you, all, for your patience with a noob.

Terry.


___
LibreOffice mailing list
LibreOffice@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/libreoffice

Re: Question about iterator management in sw/source/core/fields/cellfml.cxx

2012-02-03 Thread julien2412 
I followed the idiom you indicated, commited and pushed on master (see
9d0136679e441413b6945d2a40aa892b50ee19a8)
Thank you Stephan for the details about C++
Hope that C++11 will spread quickly because all these things aren't very
intuitive.

Julien

--
View this message in context: 
http://nabble.documentfoundation.org/Question-about-iterator-management-in-sw-source-core-fields-cellfml-cxx-tp3708331p3714286.html
Sent from the Dev mailing list archive at Nabble.com.
___
LibreOffice mailing list
LibreOffice@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/libreoffice

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

2012-02-02 Thread Stephan Bergmann 

On 02/01/2012 11:40 PM, julien2412 wrote:

Cppcheck reports this :
core/sw/source/core/fields/cellfml.cxx
970 StlMissingComparisonstyle   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
___
LibreOffice mailing list
LibreOffice@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/libreoffice

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

2012-02-02 Thread Marcel Metz 
Hello Julien

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 }

If the box that is represented by `it` should be deleted you could use.

970  it = rBoxes.erase( it );

`it` would now point to the element after the erased one. This is true
for every sequence in the standard template library (see [1] under
Expression semantics - Erase post condition).

[1] http://www.sgi.com/tech/stl/Sequence.html

regards Marcel
___
LibreOffice mailing list
LibreOffice@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/libreoffice

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

2012-02-02 Thread 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.lu...@suse.cz
___
LibreOffice mailing list
LibreOffice@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/libreoffice

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

2012-02-02 Thread Stephan Bergmann 

On 02/02/2012 10:13 AM, Marcel Metz wrote:

If the box that is represented by `it` should be deleted you could use.

970  it = rBoxes.erase( it );


Unfortunately, this is only C++11, not C++03.

Stephan
___
LibreOffice mailing list
LibreOffice@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/libreoffice

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

2012-02-02 Thread Stephan Bergmann 

On 02/02/2012 11:26 AM, Lubos Lunak wrote:

  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.


Might well be, I probably didn't notice.  And this is in no way meant to 
criticize Julien -- but I assume that's clear, anyway.



Perhaps we should agree on what the recommended way is? I personally think


Don't think its worth to go any more formal here.  I occasionally notice 
things and give comments, trying to give rationales as well.  But in the 
end, all those kinds of code are good and fine, anyway.



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


it = erase(it) is unfortunately only C++11

Stephan
___
LibreOffice mailing list
LibreOffice@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/libreoffice

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

2012-02-02 Thread Lubos Lunak 
On Thursday 02 of February 2012, Stephan Bergmann wrote:
 On 02/02/2012 11:26 AM, Lubos Lunak wrote:
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.

 Might well be, I probably didn't notice.  And this is in no way meant to
 criticize Julien -- but I assume that's clear, anyway.

  Perhaps we should agree on what the recommended way is? I personally
  think

 Don't think its worth to go any more formal here.  I occasionally notice
 things and give comments, trying to give rationales as well.  But in the
 end, all those kinds of code are good and fine, anyway.

 I definitely didn't mean to get formal here, I simply meant a description of 
how to actually handle this, as it turns up now and then, and STL doesn't 
make this trivial.

  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

 it = erase(it) is unfortunately only C++11

 Oh, it's a map. Bugger. Ok, especially in that case it might be useful to 
point out that the solution is to use a temporary iterator for removal and 
advance the real one first. I can't be the only one who'd need a while to 
figure it out otherwise.

-- 
 Lubos Lunak
 l.lu...@suse.cz
___
LibreOffice mailing list
LibreOffice@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/libreoffice

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

2012-02-02 Thread Stephan Bergmann 

On 02/02/2012 02:06 PM, Lubos Lunak wrote:

  I definitely didn't mean to get formal here, I simply meant a description of
how to actually handle this, as it turns up now and then, and STL doesn't
make this trivial.


In which case the answer should be to read Item 9 Choose carefully 
among erasing options of Meyers' Effective STL.


Stephan
___
LibreOffice mailing list
LibreOffice@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/libreoffice

Re: Question about iterator management in sw/source/core/fields/cellfml.cxx

2012-02-02 Thread julien2412 
Would this patch better ? (I kept the for loop)
diff --git a/sw/source/core/fields/cellfml.cxx
b/sw/source/core/fields/cellfml.cxx
index 33b953e..5c626dd 100644
--- a/sw/source/core/fields/cellfml.cxx
+++ b/sw/source/core/fields/cellfml.cxx
@@ -967,8 +967,8 @@ void SwTableFormula::GetBoxes( const SwTableBox
rSttBox,
 
 if( pTbl-IsHeadline( *pLine ) )
 {
-rBoxes.erase( it++ );
---it;
+SwSelBoxes::iterator toErase = it;
+rBoxes.erase( toErase );
 }
 }
 } while( sal_False );

I read that to erase a position on a iterator invalidate this iterator from
position to the end
(http://www.cplusplus.com/reference/stl/vector/erase/)
I don't know if to create this temporary variable toErase change something
or it's all the same.

Moreover, I don't know if the evolution of the standard had changed the
behavior on this point.

Any idea ?

Julien
(sorry for the nitpicking but I'd like to understand this point)

--
View this message in context: 
http://nabble.documentfoundation.org/Question-about-iterator-management-in-sw-source-core-fields-cellfml-cxx-tp3708331p3711039.html
Sent from the Dev mailing list archive at Nabble.com.
___
LibreOffice mailing list
LibreOffice@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/libreoffice

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

2012-02-01 Thread julien2412 
Hi,

Cppcheck reports this :
core/sw/source/core/fields/cellfml.cxx
970 StlMissingComparisonstyle   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 ?

patch proposed
http://nabble.documentfoundation.org/file/n3708331/sw_patch.txt sw_patch.txt 

If ok, I can commit and push it on master.

Julien

--
View this message in context: 
http://nabble.documentfoundation.org/Question-about-iterator-management-in-sw-source-core-fields-cellfml-cxx-tp3708331p3708331.html
Sent from the Dev mailing list archive at Nabble.com.
___
LibreOffice mailing list
LibreOffice@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/libreoffice