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

[Michael Stahl]

On 26/03/12 17:01, Kohei Yoshida wrote:
> 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+.

hi Bartosz,

good to hear from you again; as you can see, in this project you're not
alone in converting SvArrays, so hurry up before the competition cleans
them all up  :)

> 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. ;-)

that is only true for some of the SvArrays, namely the ones with _DEL at
the end; the plain ones don't take ownership.

> 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.

yes that kind of change just annoys reviewers :)

> 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.

always a good idea.

pushed to master:

http://cgit.freedesktop.org/libreoffice/core/commit/?id=2f657dd568ce30ec9132892080c63578e4132908