Issue 112395

Created attachment 69997 [details]
Patch to replace svarray with STL containers in highlight

Created attachment 69998 [details]
Extended patch to replace svarray.

Created attachment 70037 [details]
Patch which replace SvArray of Objects by the STL

Created attachment 70038 [details]
Patch which replace SvArray of Objects by the STL

Created attachment 70039 [details]
Patch which replace SvArray of Objects by the STL

Created attachment 70065 [details]
SvArray replace patch

Created attachment 70151 [details]
SvArray replace patch next version

finally some time to look at your work...

i'm looking at: svarray5_good.patch
(i presume it includes/supercedes all previous patches?)

some specific things:


+#ifndef INCLUDED_VECTOR
+#include <vector>
+#endif

please don't insert external header guards (the #ifndef/#endif around the include).
you have probably seen some of these in the existing code base, but nowadays
they are obsolete.
last year or so thb ran a script on the entire code to remove them, but the
script was conservative and didn't remove all of them.


+		for ( USHORT i = 0; i < aPortions.size(); i++ )

(several places)
this is wrong: USHORT is the proper index type for SvArrays (they only hold 2^16
entries), but STL vectors use size_t as the index type, and can hold 2^32 (or
even 2^64, depending on implementation) entries.
you have to replace USHORT with size_t, otherwise this can loop infinitely.


postprocess/checkdeliver/checkdeliver.pl

surely this only got in the patch by accident?


+    USHORT                  GetCount() const    { return aEntries.size(); }

same as above: must be size_t, not USHORT (you may need to investigate where
this is called, and update some more types...)


-*		SV_DECL_OBJARR(nm, AE, IS, GS)
-*		SV_IMPL_OBJARR( nm, AE )

so you've already eliminated one SvArray type?  great!


-SV_DECL_PTRARR_DEL( SfxPoolVersionArr_Impl, SfxPoolVersion_Impl*, 0, 2 )
+typedef std::vector<SfxPoolVersion_Impl*> SfxPoolVersionArr_Impl;

i'm not sure what PTRARR_DEL does exactly...
if it does something like call delete on all entries in its destructor, you'll
need to use a vector<boost::shared_ptr> instead.
please check this.


+typedef std::vector<HighlightPortion> HighlightPortions;

it seems this one is duplicate, both in .hxx and .cxx?


 #ifndef _SVSTDARR_ULONGS
 #define _SVSTDARR_ULONGS
-#include <svl/svstdarr.hxx>
 #endif

i guess you can remove the #define _SVSTDARR_ULONGS or similar as well, when you
remove the header (these are to select which SvArrays the header should declare,
and is part of the silliness of SvArrays...)


-	ASSERT( pColTbl, "Wo ist meine Color-Tabelle?" );
+	ASSERT( pColTbl, "Where's table color?" );

translating german assertions is a good idea, but in this case it probably
should be "color table", not "table color", because it's actually a lookup table
for colors.



so it seems to me you're definitely going in the right direction here.
i just wonder how big your patch will become...
i'm afraid it will soon be so big that nobody wants to look at it  :)
maybe it makes sense to split up the changes somehow?
mabe by SvArray type?  maybe by code module?
it should be possible to have several small patches because most of the uses of
the SvArrays are independent of each other.
but one patch for each array will result in thousands of patches, that's also bad.

do you know about the Mercurial Queues (MQ) extension?
makes managing multiple patches easier...

Thank you for comments to this patch.
Of course svarray5_good.patch is the latest patch which includes/supercedes all
previous patches.

I will apply all comments issues to next patch.

Is there any chance to commit this patch to the OpenOffice.org mercurial repo?

I could stop removing the svarray in this step (I successfully removed SV
OBJARR) and wait for commit this patch to OpenOfice.Org

After that I will continue work with removing obsolete svArrays.

Where may I find information about the Mercurial Queues (MQ) extension?

> Where may I find information about the Mercurial Queues (MQ) extension?

try Bryan O'Sullivan's book:
http://hgbook.red-bean.com/read/managing-change-with-mercurial-queues.html

the entire book is online, and it's good.

cd->gang65: Michael already mentioned what should be changed in your patch. I am
sorry but it's too late for OOo 3.3. We are now in a show stopper phase. We
cannot commit changes which are related to features, code rework or other bigger
changes on the code base. I have to set the target to OOo 3.4 which will be the
next version.

cd: Set mav on CC, confirm issue.

Created attachment 70229 [details]
SvArray replace patch (corrected)

The final patch still needs a review.

I'm especially concerned about the PTRARR_DEL stuff - introduction of shared_ptr
and the like is never straightforward. So while I agree with Michael here, we
should keep an eye on that.

oops, completely forgot about this :)

re: PTRARR_DEL:
it seems that replacing with shared_ptr is indeed mostly straightforward, except
in one case: if Remove() is called on a PTRARR_DEL, then it seems the removed
object will NOT be deleted, while removing an object from an STL container would
invoke the shared_ptr destructor.
(but that case doesn't seem to occur in the current patch.)

oh, and the implementation of Replace() also doesn't seem to invoke the
destructor, but i'm hoping that Replace() is not used much (and guessing that
whenever it's used on a PTRARR_DEL currently it's a memory leak ;) ).

so i think the patch is good now, thanks for fixing the problems.

you've created a cws "svarray"; please commit your patch there and push.
oh, and you should add this issue to the CWS: log into EIS, click on "tasks",
and then "add task".

if you want the ugly STOP sign on the EIS CWS page (which our tooling people
seem to think of as a great motivational tool that really makes a good
impression on new developers...) to go away then you need to edit the CWS data
in EIS (click on the "DEV300/svarray" link) and set the Release to "OOo 3.4".

Thanks mst.

I'm not experienced in CWS developing.
The svarray CWS it is my first try to commit this changes, but without success.

I think it will be much faster (and safer) to just create CWS by you Michael?

your cws is already created (so you must have done something right),
and has a "outgoing" hg repository here (you can look at it with a browser):
http://hg.services.openoffice.org/hg/cws/svarray/

basically you just need a HG checkout of this outgoing repository.

if you already have a HG checkout of the OOo master, then you can just clone
that locally.

how to get a HG checkout of OOo DEV300 is described here:
http://wiki.services.openoffice.org/wiki/Documentation/Building_Guide/Getting_the_source

to clone it (assuming the directory is named DEV300) just do "hg clone DEV300
svarray"

once it's done "cd svarray" and create the file ".hg/hgrc" with 2 lines:

[paths]
default = ssh://hg@hg.services.openoffice.org/cws/svarray


this way hg pull/hg push will operate on the outgoing repository for your CWS.

please give it another try, and let us know what doesn't work.

but i can at least try to mess around in EIS a little, to fill out some fields...

not much of an improvement:
now the CWS is "failed" because it contains an issue (this one) which is not fixed.

really, the by far most frustrating part of CWS handling is EIS...

Link to CWS with this changes:
http://hg.services.openoffice.org/cws/svarray

Please check the new commits in cws/svarray:

http://hg.services.openoffice.org/hg/cws/svarray/rev/8fbef438de59
http://hg.services.openoffice.org/hg/cws/svarray/rev/6b2f8af09ac4
http://hg.services.openoffice.org/hg/cws/svarray/rev/0b7dcacae5bf

I need also help with translating comments/messages to English near changed code.

I would like translate all German strings which is belong to touched code.

i've just looked at the new commits and they look good to me.
you even answered the obvious question "why is that not a std::stack" with a
comment :)

oh, and don't worry too much about the german comments;
if you know what they mean, you can translate them,
if you don't know what they mean, just leave them alone.

I updated following svarrays: SvShorts, SvXub_StrLens, SvBytes, SvBools.

All changes is available at:
http://hg.services.openoffice.org/hg/cws/svarray/summary

Please check this changes.

everything up to revision 81f829e22ea3 looks good to me.
except for this bit here:

    7.50 @@ -947,7 +946,8 @@
    7.51      }
    7.52  
    7.53      // remove invalid entries from kashida array
    7.54 -    aKashida.Remove( nCntKash, aKashida.Count() - nCntKash );
    7.55 +    //FIXME Is it correct?
    7.56 +    aKashida.erase( aKashida.begin() + nCntKash, aKashida.end() -
nCntKash );
    7.57  
    7.58      //
    7.59      // TAKE CARE OF WEAK CHARACTERS: WE MUST FIND AN APPROPRIATE

the second parameter to Remove is apparently the number of elements to remove.
so "Remove ( n , Count() - n )" will remove elements from "n" up
to "n + Count() - n", i.e. up to "Count()", i.e. until the end.
the second parameter to erase should be just "aKashida.end()".

I commit some new patches
http://hg.services.openoffice.org/cws/svarray/rev/df7b4d57529a
http://hg.services.openoffice.org/cws/svarray/rev/29a91d60907d
http://hg.services.openoffice.org/cws/svarray/rev/ede08423780a

I think I also resolved issue with the latest patch:
http://www.openoffice.org/issues/show_bug.cgi?id=84159

Please check the solution

Fixed in CWS "svarray":
http://hg.services.openoffice.org/cws/svarray

set to fixed

setting this one to verified for CWS svarray.

this doesn't mean that all svarrays have been replaced;
further work should happen on a new issue.

While reanimating RTF (and doc) export and import I found that this fix broke
RTF import. The SvxRTFParser (in editeng/source/rtf/svxrtf.cxx crashes while
accessing it's member 'aAttrStack' which is a
typedef std::deque< SvxRTFItemStackTypePtr > SvxRTFItemStack; that came with
this issue in cws svarray.

Reopening this issue.

Fixed in cws os144 in 
editeng\inc\editeng\svxrtf.hxx
editeng\source\rtf\rtfitem.cxx
editeng\source\rtf\svxrtf.cxx
sw\source\filter\rtf\swparrtf.cxx

Reassinged for verification

indeed, we overlooked an implementation detail of the old stack:

	AE Top() const {\
		AE pRet = 0;\
		if( SvPtrarr::Count() )\
			pRet = GetObject( SvPtrarr::Count()-1 ); \
		return pRet;\
	}\

if the stack is empty, the Top and Pop methods will return a null pointer.
on an empty STL container, back() will return some uninitialized memory, which
causes the crash.

of course returning a null pointer here is bad design anyway,
because perhaps somebody wants to store an actual null pointer in the stack...

http://hg.services.openoffice.org/cws/os144?cmd=changeset;node=82690329d55e

(btw, wouldn't it have been better to create a new issue for this regression?)

What do you think about instead using notation (example):
    SvxRTFItemStackTypePtr pTmp = aSaveStack.empty() ? 0 : aSaveStack.back();
    if( pTmp && pTmp->GetSttNodeIdx() ==
            pPam->GetPoint()->nNode.GetIndex() &&
            pTmp->GetSttCnt() == nPos )
            pTmp->GetAttrSet().ClearItem( RES_CHRATR_ESCAPEMENT );

use following:

if (!aSaveStack.empty()) 
{
    SvxRTFItemStackTypePtr pTmp = aSaveStack.back();
    if( pTmp && pTmp->GetSttNodeIdx() ==
            pPam->GetPoint()->nNode.GetIndex() &&
            pTmp->GetSttCnt() == nPos )
            pTmp->GetAttrSet().ClearItem( RES_CHRATR_ESCAPEMENT );
}

I think it is a little more clear

/editeng/source/rtf/svxrtf.cxx
Instead use of:
    876 SvxRTFItemStackType* SvxRTFParser::_GetAttrSet( int bCopyAttr )
    877 {
    878 	SvxRTFItemStackType* pAkt = aAttrStack.back();
    879 	SvxRTFItemStackType* pNew;
    880 	if( pAkt )
    881 		pNew = new SvxRTFItemStackType( *pAkt, *pInsPos, bCopyAttr );
    882 	else
    883 		pNew = new SvxRTFItemStackType( *pAttrPool, aWhichMap.GetData(),
*pInsPos );


We could use just:

    876 SvxRTFItemStackType* SvxRTFParser::_GetAttrSet( int bCopyAttr )
    877 {
    879 	SvxRTFItemStackType* pNew;
    880 	if( !aAttrStack.empty() ) {
    +++ 	        SvxRTFItemStackType* pAkt = aAttrStack.back();
    881 		pNew = new SvxRTFItemStackType( *pAkt, *pInsPos, bCopyAttr );
    882 	} else
    883 		pNew = new SvxRTFItemStackType( *pAttrPool, aWhichMap.GetData(),
*pInsPos );

@gang65:
in principle i agree with your suggestion, but i think the CWS os144 is already
in QA because it's urgent (it's not just this RTF crash, in m93 the DOC/RTF
filters are completely disabled due to some merge problem on integration...)

Verified fix in CWS os144.