Date: prev next · Thread: first prev next last
2011 Archives by date, by thread · List index


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>


Context


Privacy Policy | Impressum (Legal Info) | Copyright information: Unless otherwise specified, all text and images on this website are licensed under the Creative Commons Attribution-Share Alike 3.0 License. This does not include the source code of LibreOffice, which is licensed under the Mozilla Public License (MPLv2). "LibreOffice" and "The Document Foundation" are registered trademarks of their corresponding registered owners or are in actual use as trademarks in one or more countries. Their respective logos and icons are also subject to international copyright laws. Use thereof is explained in our trademark policy.