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 <kyosh...@novell.com> _______________________________________________ LibreOffice mailing list LibreOffice@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/libreoffice