Re: [PATCH] Convert SV_PTRARR with std::deque

On Sat, 2012-03-24 at 23:41 +0100, Bartosz wrote:
> Hi
>
> I converted the SV PTRARR to std::deque in sw component.
> Could you please check and push this path?
> This and later contributions is licensed under MPL 1.1 / GPL v3+ / LGPL v3+.

I'm not a writer guy, and my understanding with these hideous PTRARR
macros is not very good.

That said, here is my comments on your patch (since nobody has reviewed
this patch so far).

First off, it's my understanding that the old container took ownership
of the contained elements i.e. when the pointer that points to an
instance is removed from the container it would call delete on that
pointer to delete the object.  So, it's probably wise to double-check
what the old SV_PTRARR container did before pushing this change.  If it
did indeed managed the life cycle of the stored elements, then it's more
appropriate to use boost::ptr_vector or similar (not sure if there was
ptr_deque...).  If it didn't, then, ignore my suggestion. ;-)

Also, changes like the following

-                                    pCurrentTopLeftFrm = static_cast<const SwCellFrm*>( pCell );
+                                    pCurrentTopLeftFrm = static_cast< const SwCellFrm* >( pCell );

which only adds padding to the static_cast is not necessary.  Padding <>
or () is a personal preference, and we prefer not to change things just
to add or remove padding.

Lastly, it's probably a good idea to try to identify what this code is
used for and try to test this change to make sure it works (doesn't
cause crash etc).  If you've already done that, then it's all good.  If
not, I suggest you try to see how Writer uses this particular code so
that you can exercise this.

Best regards,

Kohei

-- 
Kohei Yoshida, LibreOffice hacker, Calc