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