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 2001From: Maciej Rumianowski <maciej.rumianowski@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
Attachment:
pgpWyAOZi4fU0.pgp
Description: PGP signature