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.mohrhard@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_function<bool, const ScRange*>

should be

class FindDeletedRange : public ::std::unary_function<const ScRange*,
bool> (swap the 1st and 2nd template arguments.)

The rest looks good to me.

Kohei