[Libreoffice] [REVIEW] Fix for fdo#37226

Hi there,

The attached patch fixes the crasher reported in the following bug:

https://bugs.freedesktop.org/show_bug.cgi?id=37226

and also several other usage of standard algorithms which I believe is
incorrect, since the existing code passes the result of an evaluation
which is always boolean (unless boost::bind overloads those operators to
return something else, which I doubt it does).  There were several cases
where such evaluation result was passed to standard algorithms, one of
which caused the crash.

The existing code which I replaced used boost::bind.  In my first
attempt, I tried to keep that use of boost::bind since that seems like a
pretty elegant way to create function objects in-situ.  But I didn't
find a way to achieve that after some google search, so I resorted to
just plain simple function objects to get the job done.

It's worth noting that the existing code is a result of one of the
DECLARE_LIST removal tasks.  And I noticed that it actually changed the
behavior of the code prior to the DECLARE_LIST removal.  So, some of the
change I made in my patch is based on the code prior to the removal, to
bring the original behavior back.

This code is used pretty much only by the STYLE cell function, which, I
admit, I didn't know existed. ;-)  I tested its run-time behavior after
I made my change, and confirm that it works as advertised, with no
crash, I might add.

I tried to write unit test for this, but because of the way the STYLE
function works which involves timer handler, it was rather difficult to
write unit test for it.  Hopefully my run-time test is sufficient to get
the patch included.

I appreciate your review and/or rubber-stamping. ;-)

Kohei

-- 
Kohei Yoshida, LibreOffice hacker, Calc
<kyoshida@novell.com>

From 1302c3427654c08739da69a40ac1a65b9abb9730 Mon Sep 17 00:00:00 2001
From: Kohei Yoshida <kyoshida@novell.com>
Date: Tue, 17 May 2011 11:33:17 -0400
Subject: [PATCH] fdo#37226: Use real function objects with standard algorithms.

This fixes a crasher, and also some incorrect usage of standard
algorithms, where boolean values were passed which end up doing
things that the original author probably never intended.
---
 sc/source/ui/docshell/autostyl.cxx |   66 ++++++++++++++++++++++++++---------
 1 files changed, 49 insertions(+), 17 deletions(-)

diff --git a/sc/source/ui/docshell/autostyl.cxx b/sc/source/ui/docshell/autostyl.cxx
index d05676e..10773c4 100644
--- a/sc/source/ui/docshell/autostyl.cxx
+++ b/sc/source/ui/docshell/autostyl.cxx
@@ -64,6 +64,34 @@ inline sal_uLong TimeNow()                   // Sekunden
     return (sal_uLong) time(0);
 }
 
+namespace {
+
+class FindByRange : public ::std::unary_function<ScAutoStyleData, bool>
+{
+    ScRange maRange;
+public:
+    FindByRange(const ScRange& r) : maRange(r) {}
+    bool operator() (const ScAutoStyleData& rData) const { return rData.aRange == maRange; }
+};
+
+class FindByTimeout : public ::std::unary_function<ScAutoStyleData, bool>
+{
+    sal_uLong mnTimeout;
+public:
+    FindByTimeout(sal_uLong n) : mnTimeout(n) {}
+    bool operator() (const ScAutoStyleData& rData) const { return rData.nTimeout >= mnTimeout; }
+};
+
+struct FindNonZeroTimeout : public ::std::unary_function<ScAutoStyleData, bool>
+{
+    bool operator() (const ScAutoStyleData& rData) const
+    {
+        return rData.nTimeout != 0;
+    }
+};
+
+}
+
 ScAutoStyleList::ScAutoStyleList(ScDocShell* pShell) :
     pDocSh( pShell )
 {
@@ -108,8 +136,12 @@ void ScAutoStyleList::AddEntry( sal_uLong nTimeout, const ScRange& rRange, 
const
     aTimer.Stop();
     sal_uLong nNow = TimeNow();
 
-    aEntries.erase(std::remove_if(aEntries.begin(),aEntries.end(),
-                                  boost::bind(&ScAutoStyleData::aRange,_1) == rRange));
+    // Remove the first item with the same range.
+    ::boost::ptr_vector<ScAutoStyleData>::iterator itr =
+        ::std::find_if(aEntries.begin(), aEntries.end(), FindByRange(rRange));
+
+    if (itr != aEntries.end())
+        aEntries.erase(itr);
 
     // Timeouts von allen Eintraegen anpassen
 
@@ -120,8 +152,8 @@ void ScAutoStyleList::AddEntry( sal_uLong nTimeout, const ScRange& rRange, const
     }
 
     // Einfuege-Position suchen
-    boost::ptr_vector<ScAutoStyleData>::iterator iter = 
std::find_if(aEntries.begin(),aEntries.end(),
-                                                                     
boost::bind(&ScAutoStyleData::nTimeout,_1) >= nTimeout);
+    boost::ptr_vector<ScAutoStyleData>::iterator iter =
+        ::std::find_if(aEntries.begin(), aEntries.end(), FindByTimeout(nTimeout));
 
     aEntries.insert(iter,new ScAutoStyleData(nTimeout,rRange,rStyle));
 
@@ -145,19 +177,19 @@ void ScAutoStyleList::AdjustEntries( sal_uLong nDiff )    // Millisekunden
 
 void ScAutoStyleList::ExecuteEntries()
 {
-    boost::ptr_vector<ScAutoStyleData>::iterator iter;
-    for (iter = aEntries.begin(); iter != aEntries.end();)
+    // Execute and remove all items with timeout == 0 from the begin position
+    // until the first item with non-zero timeout value.
+    ::boost::ptr_vector<ScAutoStyleData>::iterator itr = aEntries.begin(), itrEnd = aEntries.end();
+    for (; itr != itrEnd; ++itr)
     {
-        if (!iter->nTimeout)
-        {
-            pDocSh->DoAutoStyle(iter->aRange,iter->aStyle);
-            iter = aEntries.erase(iter);
-        }
-        else
-        {
-            ++iter;
-        }
+        if (itr->nTimeout)
+            break;
+
+        pDocSh->DoAutoStyle(itr->aRange, itr->aStyle);
     }
+    // At this point itr should be on the first item with non-zero timeout, or
+    // the end position in case all items have timeout == 0.
+    aEntries.erase(aEntries.begin(), itr);
 }
 
 void ScAutoStyleList::ExecuteAllNow()
@@ -174,8 +206,8 @@ void ScAutoStyleList::ExecuteAllNow()
 void ScAutoStyleList::StartTimer( sal_uLong nNow )             // Sekunden
 {
     // ersten Eintrag mit Timeout != 0 suchen
-    boost::ptr_vector<ScAutoStyleData>::iterator iter = 
std::find_if(aEntries.begin(),aEntries.end(),
-                                                                     
boost::bind(&ScAutoStyleData::nTimeout,_1) != static_cast<unsigned>(0));
+    boost::ptr_vector<ScAutoStyleData>::iterator iter =
+        ::std::find_if(aEntries.begin(),aEntries.end(), FindNonZeroTimeout());
 
     if (iter != aEntries.end())
     {
-- 
1.7.3.4