Re: [REVIEW-3-6] fix for fdo#52351, remove conditional formatting if all its cells are removed
Hi Markus,
On Sun, Aug 5, 2012 at 8:29 AM, Markus Mohrhard
markus.mohrh...@googlemail.com wrote:
[1] is mostly a fix for a strange behavior of ScRangeList which
affects also conditional formats. ScRangeList did not delete a ScRange
if UpdateReference removed all the ScRange cells. The second step of
the patch is to remove the conditional formatting if the ScRangeList
is empty after updating the range.
...
[1]
http://cgit.freedesktop.org/libreoffice/core/commit/?id=76f56b5e8d4abf17682aa75b7cf183b883809234
Regarding
iterator itr = begin();
while(itr != end())
{
if(itr-GetRange().empty())
maConditionalFormats.erase(itr++);
else
++itr;
}
that erase line causes an undefined behavior, and subtle, hard-to-find
bug later. maConditionalFormats is a boost::ptr_set which uses
std::set in its implementation. The std::set's erase call invalidates
the iterator of the element that has just been deleted, and that line
increments the iterator after it's been invalidated. Instead of doing
post-increment on the invalidated iterator, doing it this way
iterator itr = begin();
while(itr != end())
{
if(itr-GetRange().empty())
{
iterator itErase = itr;
++itr;
maConditionalFormats.erase(itErase);
}
else
++itr;
}
_should_ work.
c.f.
http://stackoverflow.com/questions/1636578/iterator-validity-after-erase-call-in-stdset
Also, this line
class FindDeletedRange : public ::std::unary_functionbool, const ScRange*
should be
class FindDeletedRange : public ::std::unary_functionconst ScRange*,
bool (swap the 1st and 2nd template arguments.)
The rest looks good to me.
Kohei
___
LibreOffice mailing list
LibreOffice@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/libreoffice
Re: [REVIEW-3-6] fix for fdo#52351, remove conditional formatting if all its cells are removed
On 08/06/2012 04:08 PM, Kohei Yoshida wrote:
Regarding
iterator itr = begin();
while(itr != end())
{
if(itr-GetRange().empty())
maConditionalFormats.erase(itr++);
else
++itr;
}
that erase line causes an undefined behavior, and subtle, hard-to-find
bug later. maConditionalFormats is a boost::ptr_set which uses
std::set in its implementation. The std::set's erase call invalidates
the iterator of the element that has just been deleted, and that line
increments the iterator after it's been invalidated.
No, the line first increments itr, then calls erase (on the old itr value).
Stephan
___
LibreOffice mailing list
LibreOffice@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/libreoffice
Re: [REVIEW-3-6] fix for fdo#52351, remove conditional formatting if all its cells are removed
On Mon, Aug 6, 2012 at 11:21 AM, Stephan Bergmann sberg...@redhat.com wrote:
On 08/06/2012 04:08 PM, Kohei Yoshida wrote:
Regarding
iterator itr = begin();
while(itr != end())
{
if(itr-GetRange().empty())
maConditionalFormats.erase(itr++);
else
++itr;
}
that erase line causes an undefined behavior, and subtle, hard-to-find
bug later. maConditionalFormats is a boost::ptr_set which uses
std::set in its implementation. The std::set's erase call invalidates
the iterator of the element that has just been deleted, and that line
increments the iterator after it's been invalidated.
No, the line first increments itr, then calls erase (on the old itr value).
Ok then. I have no problem with it.
Kohei
___
LibreOffice mailing list
LibreOffice@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/libreoffice
[REVIEW-3-6] fix for fdo#52351, remove conditional formatting if all its cells are removed
Hey, [1] is mostly a fix for a strange behavior of ScRangeList which affects also conditional formats. ScRangeList did not delete a ScRange if UpdateReference removed all the ScRange cells. The second step of the patch is to remove the conditional formatting if the ScRangeList is empty after updating the range. Regards, Markus [1] http://cgit.freedesktop.org/libreoffice/core/commit/?id=76f56b5e8d4abf17682aa75b7cf183b883809234 ___ LibreOffice mailing list LibreOffice@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/libreoffice
