Re: [Libreoffice] Removal of DECLARE_LIST() from Calc

Hi Joseph,

Thanks for looking into this.  I've done my review of the patch, and
here are my comments.

The big ones first.  With this patch, we are now inheriting directly
from std::vector, which itself does not break anything, but is not
considered a good practice.  The reason is that it exposes all of
vector's interfaces, both public and private.  It's generally not a good
practice to inherit directly from classes in the std namespace unless
it's designed to be inherited.  So, I'd much rather we define
std::vector as a data member of ScRangeList and store ScRange* there.

But since this doesn't affect functionality and is not really your fault
(the existing code was written like that), I'll make this change after
committing your change.

Now, here are some changes we need to make to fix a few issues.

ScRangeList::~ScRangeList()
{
-    for ( ScRangePtr pR = First(); pR; pR = Next() )
-        delete pR;
-}
-
-void ScRangeList::RemoveAll()
-{
-    for ( ScRangePtr pR = First(); pR; pR = Next() )
-        delete pR;
-    Clear();
+    clear();
}

We need to delete objects pointed by the stored pointers before calling
clear(), or else we'll leak memory.  So, just like the old code did, we
need to iterate through all the stored pointers and call delete on them,
then call clear() afterward.

-    BOOL bJoinedInput = FALSE;
-    for ( ScRangePtr p = First(); p && pOver; p = Next() )
+    bool bJoinedInput = false;
+
+    for ( size_t i = 0, nRanges = size(); i < nRanges; ++i )
     {

We need to keep the check for the value of pOver in the evaluation of
the for loop, or else the behavior will change.  Additionally,

             if ( bIsInList )
             { // innerhalb der Liste Range loeschen
-                Remove( nOldPos );
-                delete pOver;
+                erase( begin() + nOldPos );
                 pOver = NULL;
                 if ( nOldPos )
                     nOldPos--; // Seek richtig aufsetzen
             }

here, we need to keep the "delete pOver" line, or else we'll leak
memory.  That erase() call removes the pointer stored in the vector, but
the object pointed to by the pointer still exists.  So we need to delete
it manually like the old code does.

Also, this is a very minor point, but let's use ScRange* over
ScRangePtr.  In fact, I'll probably remove this ScRangePtr typedef since
it's meaningless and I prefer seeing the '*' for pointer types.

On Wed, 2010-12-08 at 22:55 -0800, Joseph Powers wrote:
> I'm converting ScRangeList from "DECLARE_LIST( ScRangeListBase,
> ScRange* ) " to "::STD::vector< ScRange*> ScRangeListBase"
>
>
> The most of the code boring and unlikely to cause issues; however,
> some areas need further review:
>
>
> sc/source/core/tool/rangelst.cxx <- This is the main class ScRangeList
> and a child of ScRangeListBase. If this is wrong, we're in trouble.
> - I changed GetCellCount() from ULONG to size_t which is more correct.
> Can someone verify that this doesn't break 64bit builds?

I haven't tested it yet, but I can't see how using size_t would break
64-bit builds, since I use size_t a lot in other areas. 

> sc/source/ui/miscdlgs/acredin.cxx
> - Re-wrote some logic to not use iterators. A review would be nice.

It looks good to me.

> sc/source/ui/unoobj/cellsuno.cxx
> - The is huge with lots of changes.

It looks ok to me.

BTW,

     switch (pToken->GetType())
     {
-        case svSingleRef:
+    case svSingleRef:
         {
             const ScSingleRefData& rData = pToken->GetSingleRef();
             nMinCol = rData.nCol;

Let's not make these changes since this is just an aesthetic issue, and
I happen to indent case from switch myself.

Also

-        default:
-            ;
+    default:
+        break; 

The original code is okay, and there is nothing to fix here.  So, let's
leave this alone.  You'll see code like this in many places, and it was
done to suppress compiler warnings.  You can think of this as an idiom
of sort.

> I now pass a few size_t values to sal_Int32 & sal_uInt32 parameters.
> The Mac build only supports 32bits currently so this isn't an issue. I
> need someone with a 64bit build machine to review this patch.

I wouldn't worry about this.  We do lots of this in other places
anyway. ;-)

> ps: Sorry about the size... but I had to change everything in one pass
> or not at all.

Not a problem. :-)

Ok.  Now I've committed your patch and committed my own fix afterward.
Thanks a lot for getting this piece done!

Kohei 

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