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 _______________________________________________ LibreOffice mailing list [email protected] http://lists.freedesktop.org/mailman/listinfo/libreoffice
