Re: [Libreoffice] [PATCH] Get rid of SvULongs in calc

Hi Maciej,

First of all, thanks for submitting your patches.  I've done my review
of both of your patches, and I'll leave my comments below.

First, for your svl patch:

1) in SfxIntegerListItem::GetList(), you do

+        rList.insert( aIt, m_aList[n] );

but you can simply call push_back here to append new data to the end of
the vector.  Also, let's use pre-increment on iterators in the "for"
loop there i.e. instead of doing aIt++, do ++aIt.  Post-increment on
iterators is generally more expensive than pre-increment, so it's best
to stick with pre-increments.

2) Your new constructors take arrays of different integer type; one
takes std::vector<sal_uLong> while the other takes
uno::Sequence<sal_Int32>.  Let's just stick with one type and use
sal_Int32 in both cases since that's the integer type that this class
uses internally.

And for your sc patch:

3) In ScTabViewShell::Execute() you do

-            SvULongs aIndexList( 4, 4 );
+            ::std::vector < sal_uLong > aIndexList( 4, 4 );

but this actually changes the behavior of the code.  The arguments to
SvULong's constructor controls how the internal array is laid out during
initialization but it doesn't populate the container with data.
Vector's ctor arguments OTOH populate the container with data.  So, your
new code initializes aIndexList with 4 elements of value 4, which is not
what the code intends to do.  You can simply do without passing any
arguments to aIndexList there.

4) Last but not least, please state that you are submitting your patches
under LGPLv3+/MPL 1.1.

That's all I can think of.  Let's get these points revised, and I'll
push the changes to master.

Best,

Kohei

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