[Libreoffice] [PATCH] Get rid of SvULongs in calc
Maciej Rumianowski
maciej.rumianowski at gmail.com
Tue Jul 19 02:06:49 PDT 2011
Hi Kohei,
Thanks for review!
> 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.
Done, and with push_back we don't need iterator.
> 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.
Done and checked in other places to use right type.
> 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.
Done, Now I understand the SvULong's constructor. Also changed type to
sal_uInt32.
> 4) Last but not least, please state that you are submitting your patches
> under LGPLv3+/MPL 1.1.
Of course and future also.
Best Regards,
Maciej
More information about the LibreOffice
mailing list