[Libreoffice] [PATCH] Replace SvULongs with vector and code clean up part 1
Eike Rathke
ooo at erack.de
Sat Aug 6 08:35:01 PDT 2011
Hi Maciej,
On Saturday, 2011-08-06 15:32:52 +0200, Maciej Rumianowski wrote:
> With this patch comes some questions:
> > @@ -159,16 +159,10 @@ SvxNumberFormatShell::~SvxNumberFormatShell()
> > // Hinzugefuegte Formate sind nicht gueltig:
> > // => wieder entfernen:
> >
> > - for ( sal_uInt16 i = 0; i < aAddList.Count(); ++i )
> > - pFormatter->DeleteEntry( aAddList[i] );
> > + for ( std::vector<sal_uInt32>::const_iterator it =
> > aAddList.begin(); it != aAddList.end(); ++it )
> > + pFormatter->DeleteEntry( *it );
> > }
> >
> > - //--------------------------------
> > - // Add-/Remove-Listen leerraeumen:
> > - //--------------------------------
> > - aAddList.Remove( 0, aAddList.Count() );
> > - aDelList.Remove( 0, aAddList.Count() );
>
> 1. It is not necessary to explicitly clear vectors?
As the vector is a member of SvxNumberFormatShell it is destroyed in
SvxNumberFormatShell's dtor anyway, no need to explicitly clear it.
Which also wasn't necessary for the array members. Sometimes the order
in which vector members are removed from different vectors is important,
but not here. The code is wrong anyway, as it uses aAddList.Count() for
both, aAddList and aDelList ...
> > @@ -1279,21 +1272,6 @@ void
> > SvxNumberFormatShell::MakePrevStringFromVal(
> > pFormatter->GetPreviewString( rFormatStr, nValue, rPreviewStr,
> > &rpFontColor, eCurLanguage );
> > }
> >
> > -/*************************************************************************
> > -#* Member: GetComment4Entry Datum:30.10.97
> > -#*------------------------------------------------------------------------
> > -#*
> > -#* Klasse: SvxNumberFormatShell
> > -#*
> > -#* Funktion: Liefert den Kommentar fuer einen gegebenen
> > -#* Eintrag zurueck.
> > -#*
> > -#* Input: Nummer des Eintrags
> > -#*
> > -#* Output: Kommentar-String
> > -#*
> > -#************************************************************************/
> > -
> > void SvxNumberFormatShell::SetComment4Entry(short nEntry,String
> > aEntStr)
> > {
> > SvNumberformat *pNumEntry;
>
> 2. Is it useful to translate comments like this? and convert to actual
> documentation style?
Yes, but move the documentation to the header file instead, there it is
more useful for someone using the class. However, for reviews it is
better to separate patches into actual code changes and
translation/documentation changes.
> > @@ -228,9 +229,9 @@ private:
> > String aValStr;
> > double nValNum;
> > sal_Bool bUndoAddList;
> > - SvULongs aAddList;
> > - SvULongs aDelList;
> > - SvULongs aCurEntryList;
> > + std::vector<sal_uInt32> aAddList;
> > + std::vector<sal_uInt32> aDelList;
> > + std::vector<sal_uInt32> aCurEntryList;
> 3. For code formating tabs are okay? I have used spaces, but previous
> there were tabs.
Spaces please.
> > @@ -229,21 +224,18 @@ void SvxNumberFormatShell::FormatChanged( sal_uInt16 nFmtLbPos,
> > String& rPreviewStr,
> > Color*& rpFontColor )
> > {
> > - //nCurFormatKey = pCurFmtTable->GetKey( pCurFmtTable->GetObject( nFmtLbPos ) );
> > -
> 4. If there is commented code should it be removed?
Depends on. Most commented code like that are remains of old times,
sometimes they indicate what was changed and why, but most times they
are useless, as is the case here.
> > @@ -1325,7 +1288,7 @@ String SvxNumberFormatShell::GetComment4Entry(short nEntry)
> > if(nEntry < 0)
> > return String();
> >
> > - if(nEntry<aCurEntryList.Count())
> > + if(nEntry < (short)aCurEntryList.size())
> > {
>
> 5. Should short type be replaced with sal_Int16 or more appropriate type?
May, but note that in that case all callers have to be adapted as well
in case on some platform sal_Int16 differs from short. Anyway, when
changing I'd go for sal_Int32 instead.
> >From 38fff431d906bfaf39848e443709b7a957d238b2 Mon Sep 17 00:00:00 2001
> From: Maciej Rumianowski <maciej.rumianowski at gmail.com>
> Date: Sat, 6 Aug 2011 15:27:06 +0200
> Subject: [PATCH] Replace SvULongs with vector and code clean up
> --- a/svx/source/items/numfmtsh.cxx
> +++ b/svx/source/items/numfmtsh.cxx
> @@ -177,20 +171,21 @@ SvxNumberFormatShell::~SvxNumberFormatShell()
>
> sal_uInt32 SvxNumberFormatShell::GetUpdateDataCount() const
> {
> - return aDelList.Count();
> + return aDelList.size();
Here change also the method's return type to size_t, also in the header
file. Make sure to change the callers accordingly.
> void SvxNumberFormatShell::GetUpdateData( sal_uInt32* pDelArray, const sal_uInt32 nSize )
> {
> - const sal_uInt32 nCount = aDelList.Count();
> + const sal_uInt32 nListSize = aDelList.size();
Also here, please use size_t consistently for vector::size().
> + std::vector<sal_uInt32>::const_iterator it;
>
> - DBG_ASSERT( pDelArray && ( nSize == nCount ), "Array nicht initialisiert!" );
> + DBG_ASSERT( pDelArray && ( nSize == nListSize ), "Array is not initialized" );
>
> - if ( pDelArray && ( nSize == nCount ) )
> - for ( sal_uInt16 i = 0; i < aDelList.Count(); ++i )
> - *pDelArray++ = aDelList[i];
> + if ( pDelArray && ( nSize == nListSize ) )
> + for ( it = aDelList.end(); it != aDelList.end(); ++it )
That should be
for ( it = aDelList.begin(); it != aDelList.end(); ++it )
instead. Note begin() instead of end().
> @@ -229,21 +224,18 @@ void SvxNumberFormatShell::FormatChanged( sal_uInt16 nFmtLbPos,
> String& rPreviewStr,
> Color*& rpFontColor )
> {
> - //nCurFormatKey = pCurFmtTable->GetKey( pCurFmtTable->GetObject( nFmtLbPos ) );
> -
> - if(nFmtLbPos<aCurEntryList.Count())
> + if( nFmtLbPos < aCurEntryList.size() )
This is a bit awkward. To prevent compiler diagnosis bailing out when
--enable-werror is active (you did configure with that, didn't you?) for
the size mismatch, the nFmtLbPos may have to be casted to size_t.
> @@ -263,20 +255,20 @@ sal_Bool SvxNumberFormatShell::AddFormat( String& rFormat, xub_StrLen& rErrPos,
> {
> if ( IsRemoved_Impl( nAddKey ) )
> {
> - // Key suchen und loeschen
> sal_Bool bFound = sal_False;
> - sal_uInt16 nAt = 0;
> + std::vector<sal_uInt32>::iterator nAt = aDelList.begin();
> + std::vector<sal_uInt32>::iterator it;
Remove that line and ...
>
> - for ( sal_uInt16 i = 0; !bFound && i < aDelList.Count(); ++i )
> + for ( it = aDelList.begin(); !bFound && it != aDelList.end(); ++it )
... use
for ( std::vector<sal_uInt32>::iterator it( aDelList.begin()); !bFound && it != aDelList.end(); ++it )
instead, so it is a) local to the loop and b) gives the compiler a hint
that it doesn't need to instanciate a default iterator that will be
assigned a value immediately after. I guess most compilers should get
that right anyway, but..
> {
> - if ( aDelList[i] == nAddKey )
> + if ( *it == nAddKey )
> {
> bFound = sal_True;
> - nAt = i;
> + nAt = it;
> }
> }
> DBG_ASSERT( bFound, "Key not found" );
> - aDelList.Remove( nAt );
> + aDelList.erase( nAt );
Hmm.. I'd say the original code was wrong, when nAddKey was not found in
aDelList it removed the first element anyway. That should be
if (bFound)
aDelList.erase( nAt );
instead.
> @@ -326,32 +318,32 @@ sal_Bool SvxNumberFormatShell::RemoveFormat( const String& rFormat,
> {
> [...]
>
> - DBG_ASSERT( nDelKey != NUMBERFORMAT_ENTRY_NOT_FOUND, "Eintrag nicht gefunden!" );
> - DBG_ASSERT( !IsRemoved_Impl( nDelKey ), "Eintrag bereits geloescht!" );
> + DBG_ASSERT( nDelKey != NUMBERFORMAT_ENTRY_NOT_FOUND, "Entry not found!" );
> + DBG_ASSERT( !IsRemoved_Impl( nDelKey ), "Entry already removed!" );
>
> if ( (nDelKey != NUMBERFORMAT_ENTRY_NOT_FOUND) && !IsRemoved_Impl( nDelKey ) )
> {
> - aDelList.Insert( nDelKey, aDelList.Count() );
> + aDelList.push_back( nDelKey );
>
> if ( IsAdded_Impl( nDelKey ) )
> {
> - // Key suchen und loeschen
> - sal_Bool bFound = sal_False;
> - sal_uInt16 nAt = 0;
> + bool bFound = false;
> + std::vector<sal_uInt32>::iterator nAt = aAddList.begin();
> + std::vector<sal_uInt32>::iterator it;
>
> - for ( sal_uInt16 i = 0; !bFound && i < aAddList.Count(); ++i )
> + for ( it = aAddList.begin(); !bFound && it != aAddList.end(); ++it )
Same here for the iterator as above.
> {
> - if ( aAddList[i] == nDelKey )
> + if ( *it == nDelKey )
> {
> - bFound = sal_True;
> - nAt = i;
> + bFound = true;
> + nAt = it;
> }
> }
> DBG_ASSERT( bFound, "Key not found" );
> - aAddList.Remove( nAt );
> + aAddList.erase( nAt );
Same here as above,
if (bFound)
aAddList.erase( nAt );
> @@ -721,11 +712,11 @@ short SvxNumberFormatShell::FillEListWithFormats_Impl( SvStrings& rList,short nS
>
> if ( nNFEntry == nCurFormatKey )
> {
> - nSelPos = ( !IsRemoved_Impl( nNFEntry ) ) ? aCurEntryList.Count() : SELPOS_NONE;
> + nSelPos = ( !IsRemoved_Impl( nNFEntry ) ) ? aCurEntryList.size() : SELPOS_NONE;
> }
>
> rList.Insert( pStr,rList.Count());
> - aCurEntryList.Insert( nNFEntry, aCurEntryList.Count() );
> + aCurEntryList.erase( aCurEntryList.begin()+nNFEntry, aCurEntryList.end() );
Erm.. erase instead of insert?? Should be
aCurEntryList.push_back( nNFEntry );
> @@ -1071,23 +1062,23 @@ short SvxNumberFormatShell::FillEListWithUserCurrencys( SvStrings& rList,short n
> if(bFlag)
> {
> rList.Insert(new String(aInsStr),nPos);
> - aCurEntryList.Insert( NUMBERFORMAT_ENTRY_NOT_FOUND, nPos++);
> + aCurEntryList.insert( aCurEntryList.begin()+nPos++, NUMBERFORMAT_ENTRY_NOT_FOUND);
Make that
aCurEntryList.insert( aCurEntryList.begin() + (nPos++), NUMBERFORMAT_ENTRY_NOT_FOUND);
instead to disambiguate.
> + aCurEntryList.insert( aCurEntryList.begin()+nPos++, aKeyList[j]);
Same here
+ aCurEntryList.insert( aCurEntryList.begin() + (nPos++), aKeyList[j]);
> - for(i=0;i<aKeyList.Count();i++)
> + for( i=0; i < aKeyList.size(); ++i)
Note that i,nPos,nOldlistCount were sal_uInt16 and now should be size_t.
> @@ -1189,8 +1180,9 @@ void SvxNumberFormatShell::GetPreviewString_Impl( String& rString, Color*& rpCol
> sal_Bool SvxNumberFormatShell::IsRemoved_Impl( sal_uInt32 nKey )
> {
> sal_Bool bFound = sal_False;
> - for ( sal_uInt16 i = 0; !bFound && i < aDelList.Count(); ++i )
> - if ( aDelList[i] == nKey )
> + std::vector<sal_uInt32>::const_iterator it;
> + for ( it = aDelList.begin(); !bFound && it != aDelList.end(); ++it )
The local to loop iterator again.
> + if ( *it == nKey )
> bFound = sal_True;
> return bFound;
> }
> @@ -1200,8 +1192,9 @@ sal_Bool SvxNumberFormatShell::IsRemoved_Impl( sal_uInt32 nKey )
> sal_Bool SvxNumberFormatShell::IsAdded_Impl( sal_uInt32 nKey )
> {
> sal_Bool bFound = sal_False;
> - for ( sal_uInt16 i = 0; !bFound && i < aAddList.Count(); ++i )
> - if ( aAddList[i] == nKey )
> + std::vector<sal_uInt32>::const_iterator it;
> + for ( it = aAddList.begin(); !bFound && it != aAddList.end(); ++it )
ditto
> + if ( *it == nKey )
> bFound = sal_True;
> return bFound;
> }
> String SvxNumberFormatShell::GetComment4Entry(short nEntry)
> {
> const SvNumberformat *pNumEntry;
> @@ -1325,7 +1288,7 @@ String SvxNumberFormatShell::GetComment4Entry(short nEntry)
> if(nEntry < 0)
> return String();
>
> - if(nEntry<aCurEntryList.Count())
> + if(nEntry < (short)aCurEntryList.size())
When casting for comparison, always cast the smaller size to the larger
size, else theoretically integer truncation may occur. Wouldn't happen
here given the current implementation, but it is good practice. And
please use C++ casts, like so
+ if(static_cast<size_t>(nEntry) < aCurEntryList.size())
> short SvxNumberFormatShell::GetCategory4Entry(short nEntry)
> {
> const SvNumberformat *pNumEntry;
> if(nEntry<0) return 0;
>
> - if(nEntry<aCurEntryList.Count())
> + if(nEntry < (short)aCurEntryList.size())
Same here.
> sal_Bool SvxNumberFormatShell::GetUserDefined4Entry(short nEntry)
> {
> const SvNumberformat *pNumEntry;
> if(nEntry<0) return 0;
> - if(nEntry<aCurEntryList.Count())
> + if(nEntry < (short)aCurEntryList.size())
And here.
> short SvxNumberFormatShell::GetListPos4Entry(sal_uInt32 nIdx)
> {
> short nSelP=SELPOS_NONE;
> - if( aCurEntryList.Count() <= 0x7fff )
> + if( aCurEntryList.size() <= 0x7fff )
Oh, lovely, hard coded size limits ;-)
> {
> - for(short i=0;i<aCurEntryList.Count();i++)
> + for(short i = 0; i < (short)aCurEntryList.size(); i++)
Make that
for(size_t i = 0; i < aCurEntryList.size(); i++)
instead.
Thanks
Eike
--
PGP/OpenPGP/GnuPG encrypted mail preferred in all private communication.
Key ID: 0x293C05FD - 997A 4C60 CE41 0149 0DB3 9E96 2F1A D073 293C 05FD
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 198 bytes
Desc: not available
URL: <http://lists.freedesktop.org/archives/libreoffice/attachments/20110806/8ff7008b/attachment.pgp>
More information about the LibreOffice
mailing list