[PUSHED] Re: [PATCH] Convert SV_PTRARR with std::deque
mstahl at redhat.com
Mon Apr 2 14:36:23 PDT 2012
On 26/03/12 17:01, Kohei Yoshida wrote:
> On Sat, 2012-03-24 at 23:41 +0100, Bartosz wrote:
>> 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+.
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:
More information about the LibreOffice