Date: prev next · Thread: first prev next last
2011 Archives by date, by thread · List index


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@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


Context


Privacy Policy | Impressum (Legal Info) | Copyright information: Unless otherwise specified, all text and images on this website are licensed under the Creative Commons Attribution-Share Alike 3.0 License. This does not include the source code of LibreOffice, which is licensed under the Mozilla Public License (MPLv2). "LibreOffice" and "The Document Foundation" are registered trademarks of their corresponding registered owners or are in actual use as trademarks in one or more countries. Their respective logos and icons are also subject to international copyright laws. Use thereof is explained in our trademark policy.