[PUSHED] Re: [PATCH] Convert SV_PTRARR with std::deque
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_castconst 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
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_castconst 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 ___ LibreOffice mailing list LibreOffice@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/libreoffice
Re: [PATCH] Convert SV_PTRARR with std::deque
On Mon, 2012-03-26 at 11:01 -0400, Kohei Yoshida wrote: 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. ;-) In addition to that, it's also possible that the objects that these pointers point to should not be managed by this container, in which case, use of std::deque or std::vector without explicit deleting is just fine. Double-checking the code where it's used seems to suggest that that may be the case, but again I'm not a Writer guy so don't take my word for it. ;-) One thing that you might try is (if the above is true) to store const pointer instead of a plain pointer to see if that compiles, and if it does, then you can remove those const_cast that you added when inserting pointers to the new container. Regards, Kohei ___ LibreOffice mailing list LibreOffice@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/libreoffice
[PATCH] Convert SV_PTRARR with std::deque
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+.
Best Regards
Bartosz
From a0291c115fa588b7e0d2faeb0df38c7a8f694004 Mon Sep 17 00:00:00 2001
From: Bartosz Kosiorek gan...@poczta.onet.pl
Date: Sat, 24 Mar 2012 23:01:06 +0100
Subject: [PATCH] Convert SV_PTRARRAY to ::std::deque
---
sw/inc/crsrsh.hxx|1 -
sw/inc/tblsel.hxx|3 ++-
sw/source/core/crsr/trvltbl.cxx |2 +-
sw/source/core/frmedt/fetab.cxx | 14 +++---
sw/source/core/frmedt/tblsel.cxx | 33 +++--
5 files changed, 25 insertions(+), 28 deletions(-)
diff --git a/sw/inc/crsrsh.hxx b/sw/inc/crsrsh.hxx
index fba2e68..0c07972 100644
--- a/sw/inc/crsrsh.hxx
+++ b/sw/inc/crsrsh.hxx
@@ -69,7 +69,6 @@ class SwTxtINetFmt;
class SwFmtINetFmt;
class SwTxtAttr;
class SwTableBox;
-class SwCellFrms;
class SwTOXMark;
class SwRedline;
class IBlockCursor;
diff --git a/sw/inc/tblsel.hxx b/sw/inc/tblsel.hxx
index 5ecc65b..5725956 100644
--- a/sw/inc/tblsel.hxx
+++ b/sw/inc/tblsel.hxx
@@ -34,6 +34,7 @@
#include swdllapi.h
#include map
+#include deque
class SwCrsrShell;
class SwCursor;
@@ -49,7 +50,7 @@ class SwTable;
class SwUndoTblMerge;
class SwCellFrm;
-SV_DECL_PTRARR( SwCellFrms, SwCellFrm*, 16 )
+typedef ::std::deque SwCellFrm* SwCellFrms;
class SwSelBoxes : private std::mapsal_uLong, SwTableBox*
diff --git a/sw/source/core/crsr/trvltbl.cxx b/sw/source/core/crsr/trvltbl.cxx
index cae5aa8..44a2b95 100644
--- a/sw/source/core/crsr/trvltbl.cxx
+++ b/sw/source/core/crsr/trvltbl.cxx
@@ -217,7 +217,7 @@ sal_Bool SwCrsrShell::_SelTblRowOrCol( bool bRow, bool
bRowSimple )
static_castconst SwCellFrm*(pEndFrm),
aBoxes, bSelectUp ? 0 : aCells, eType );
-if( aBoxes.empty() || ( !bSelectUp 4 != aCells.Count() ) )
+if( aBoxes.empty() || ( !bSelectUp 4 != aCells.size() ) )
return sal_False;
if ( bSelectUp )
diff --git a/sw/source/core/frmedt/fetab.cxx b/sw/source/core/frmedt/fetab.cxx
index 3d3a1ae..9da85cd 100644
--- a/sw/source/core/frmedt/fetab.cxx
+++ b/sw/source/core/frmedt/fetab.cxx
@@ -2345,12 +2345,12 @@ sal_Bool lcl_IsFormulaSelBoxes( const SwTable rTbl,
const SwTblBoxFormula rFml
for( SwSelBoxes::reverse_iterator it = aBoxes.rbegin(); it !=
aBoxes.rend(); ++it )
{
SwTableBox* pBox = it-second;
-sal_uInt16 i;
-for( i = 0; i rCells.Count(); ++i )
-if( rCells[ i ]-GetTabBox() == pBox )
+SwCellFrms::iterator iC;
+for( iC = rCells.begin(); iC != rCells.end(); ++iC )
+if( (*iC)-GetTabBox() == pBox )
break; // found
-if( i == rCells.Count() )
+if( iC == rCells.end() )
return sal_False;
}
@@ -2371,7 +2371,7 @@ sal_Bool SwFEShell::GetAutoSum( String rFml ) const
if( ::GetAutoSumSel( *this, aCells ))
{
sal_uInt16 nW = 0, nInsPos = 0;
-for( sal_uInt16 n = aCells.Count(); n; )
+for( size_t n = aCells.size(); n; )
{
SwCellFrm* pCFrm = aCells[ --n ];
sal_uInt16 nBoxW = pCFrm-GetTabBox()-IsFormulaOrValueBox();
@@ -2393,7 +2393,7 @@ sal_Bool SwFEShell::GetAutoSum( String rFml ) const
{
nW = RES_BOXATR_VALUE;
// restore previous spaces!
-for( sal_uInt16 i = aCells.Count(); n+1 i; )
+for( size_t i = aCells.size(); n+1 i; )
{
String sTmp( String::CreateFromAscii(
RTL_CONSTASCII_STRINGPARAM( | )) );
@@ -2431,7 +2431,7 @@ sal_Bool SwFEShell::GetAutoSum( String rFml ) const
nW = RES_BOXATR_VALUE;
rFml.Erase( nInsPos );
// restore previous spaces!
-for( sal_uInt16 i = aCells.Count(); n+1 i; )
+for( size_t i = aCells.size(); n+1 i; )
{
String sTmp( String::CreateFromAscii(
RTL_CONSTASCII_STRINGPARAM( | )) );
diff --git a/sw/source/core/frmedt/tblsel.cxx b/sw/source/core/frmedt/tblsel.cxx
index 2a4123e..c6a8b64 100644
--- a/sw/source/core/frmedt/tblsel.cxx
+++ b/sw/source/core/frmedt/tblsel.cxx
@@ -74,9 +74,6 @@
#undef DEL_EMPTY_BOXES_AT_START_AND_END
#define DEL_ALL_EMPTY_BOXES
-
-SV_IMPL_PTRARR( SwCellFrms, SwCellFrm* )
-
struct _CmpLPt
{
Point aPos;
@@ -358,7 +355,7 @@ void GetTblSel( const SwLayoutFrm* pStart, const
SwLayoutFrm* pEnd,
aTopLeft.X() aCurrentTopLeft.X() ) )
{
aCurrentTopLeft = aTopLeft;
-
