Re: [Libreoffice] [PATCH] convert SdCustomShow from tools/list to vector

Hi Noel,

On Fri, 2012-01-27 at 15:05 +0200, Noel Grandin wrote:

> Attached patch converts ScCustomShow to use std::vector.

I suppose you meant S*d*CustomShow. Anyway that typo accidentally got my
attention. :-)

> Not exactly sure how these kinds of classes that extend tools/List 
> should be converted.
> I exposed an accessor method PagesVector(), but I'm happy to modify the 
> patch if there is a better idiom.

Each case is different, and IMO in this particular case, what you did is
just fine since SdCustomShow is not used too widely and the way it's
used is pretty simple.

That said, there are a few changes that I would make.

1) I would put the typedef inside the SdCustomShow scope with public
visibility.  I don't see why that has to be in the global scope.  And
once it's in the parent class scope, you can shorten the name a bit i.e.
no more Sd prefix etc since you already get that from the parent class
name.

2) In the copy constructor you should use the initializer to copy
maPages i.e. maPages(rShow.maPages) which is more efficient than using
assignment in the body of the constructor.

3) Now, this one may be my personal preference, but I wouldn't blindly
use inline methods unless the profiler tells you otherwise.  In reality
it's a myth that inline method always gives you better performance
(sometimes it becomes worse), and it prolongs the build time when you
want to insert a debug code in that method because the body of that
method is exposed in the header.  So, to me, the cost outweighs the
potential benefit.

4) Last point.  List takes ownership of the contained objects, which
means it automatically deletes those objects when destroyed.  When you
use vector, that convenience is gone, so you'll need to manually delete
the contained objects.  Or, in this case it's probably better to use
ptr_vector instead.

Best,

Kohei

-- 
Kohei Yoshida, LibreOffice hacker, Calc