Issue 112395 - svl: replace svarray with STL containers
Summary: svl: replace svarray with STL containers
Status: CLOSED FIXED
Alias: None
Product: General
Classification: Code
Component: code (show other issues)
Version: DEV300m84
Hardware: All All
: P4 Trivial with 2 votes (vote)
Target Milestone: 3.4.0
Assignee: michael.ruess
QA Contact: issues@framework
URL: http://svn.services.openoffice.org/op...
Keywords:
Depends on: 116055
Blocks:
  Show dependency tree
 
Reported: 2010-06-14 18:18 UTC by gang65
Modified: 2017-05-20 10:22 UTC (History)
5 users (show)

See Also:
Issue Type: PATCH
Latest Confirmation in: ---
Developer Difficulty: ---


Attachments
Patch to replace svarray with STL containers in highlight (8.94 KB, text/plain)
2010-06-14 18:26 UTC, gang65
no flags Details
Extended patch to replace svarray. (14.12 KB, text/plain)
2010-06-14 22:14 UTC, gang65
no flags Details
Patch which replace SvArray of Objects by the STL (21.10 KB, text/plain)
2010-06-16 19:02 UTC, gang65
no flags Details
Patch which replace SvArray of Objects by the STL (21.10 KB, patch)
2010-06-16 19:03 UTC, gang65
no flags Details | Diff
Patch which replace SvArray of Objects by the STL (21.10 KB, patch)
2010-06-16 19:05 UTC, gang65
no flags Details | Diff
SvArray replace patch (27.35 KB, patch)
2010-06-17 23:26 UTC, gang65
no flags Details | Diff
SvArray replace patch next version (31.91 KB, patch)
2010-06-22 18:01 UTC, gang65
no flags Details | Diff
SvArray replace patch (corrected) (40.79 KB, patch)
2010-06-25 20:34 UTC, gang65
no flags Details | Diff

Note You need to log in before you can comment on or make changes to this issue.
Description gang65 2010-06-14 18:18:45 UTC
There are lots of SvArray containers defined in the files:
http://svn.services.openoffice.org/opengrok/xref/DEV300_m82/svl/inc/svl/svarray.h
xx
http://svn.services.openoffice.org/opengrok/xref/DEV300_m82/svl/source/memtools/s
varray.cxx

these are defined as a bunch of ugly macros, and were designed in the
early 90ies when STL did not exist.

The svarray must be replaced with STL containers.
Comment 1 gang65 2010-06-14 18:26:47 UTC
Created attachment 69997 [details]
Patch to replace svarray with STL containers in highlight
Comment 2 gang65 2010-06-14 22:14:55 UTC
Created attachment 69998 [details]
Extended patch to replace svarray.
Comment 3 gang65 2010-06-16 19:02:49 UTC
Created attachment 70037 [details]
Patch which replace SvArray of Objects by the STL
Comment 4 gang65 2010-06-16 19:03:02 UTC
Created attachment 70038 [details]
Patch which replace SvArray of Objects by the STL
Comment 5 gang65 2010-06-16 19:05:06 UTC
Created attachment 70039 [details]
Patch which replace SvArray of Objects by the STL
Comment 6 gang65 2010-06-17 23:26:18 UTC
Created attachment 70065 [details]
SvArray replace patch
Comment 7 gang65 2010-06-22 18:01:09 UTC
Created attachment 70151 [details]
SvArray replace patch next version
Comment 8 mst.ooo 2010-06-23 18:43:39 UTC
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...
Comment 9 gang65 2010-06-24 08:00:24 UTC
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.
Comment 10 gang65 2010-06-24 08:11:57 UTC
Where may I find information about the Mercurial Queues (MQ) extension?
Comment 11 mst.ooo 2010-06-24 10:09:24 UTC
> 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.
Comment 12 carsten.driesner 2010-06-24 14:17:47 UTC
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.
Comment 13 gang65 2010-06-25 20:34:44 UTC
Created attachment 70229 [details]
SvArray replace patch (corrected)
Comment 14 Mathias_Bauer 2010-07-28 17:43:57 UTC
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.
Comment 15 mst.ooo 2010-07-29 12:18:25 UTC
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".
Comment 16 gang65 2010-07-29 16:06:12 UTC
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?


Comment 17 mst.ooo 2010-07-30 13:34:41 UTC
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...
Comment 18 mst.ooo 2010-07-30 13:48:32 UTC
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...
Comment 19 gang65 2010-09-17 18:09:45 UTC
Link to CWS with this changes:
http://hg.services.openoffice.org/cws/svarray
Comment 20 gang65 2010-09-22 07:47:47 UTC
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.
Comment 21 mst.ooo 2010-09-22 11:37:01 UTC
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.
Comment 22 gang65 2010-10-06 08:04:24 UTC
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.
Comment 23 mst.ooo 2010-10-06 17:03:03 UTC
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()".
Comment 25 gang65 2010-10-20 14:43:25 UTC
Fixed in CWS "svarray":
http://hg.services.openoffice.org/cws/svarray
Comment 26 gang65 2010-10-20 14:44:01 UTC
set to fixed
Comment 27 mst.ooo 2010-10-25 12:02:15 UTC
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.
Comment 28 Oliver Specht 2010-11-18 09:45:04 UTC
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.
Comment 29 Oliver Specht 2010-11-19 13:25:39 UTC
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

Comment 30 Oliver Specht 2010-11-19 13:48:16 UTC
Reassinged for verification
Comment 31 mst.ooo 2010-11-19 13:50:07 UTC
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?)
Comment 32 gang65 2010-11-19 15:14:26 UTC
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
Comment 33 gang65 2010-11-19 15:21:00 UTC
/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 );

Comment 34 mst.ooo 2010-11-19 16:54:38 UTC
@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...)
Comment 35 michael.ruess 2010-11-23 16:20:24 UTC
Verified fix in CWS os144.