Re: [Libreoffice] [PATCH] Changed String to OUString in funcdesc.hxx

I have now added a function ResId::toString which returns an OUString
from a ResId to tools/inc/resid.hxx, and changed the old workaround to
use this function. I also have changed OUString to OUStringBuffer in
getSignature() and GetParamList(), and modified the operations in
these functions accordingly.

Attached two patches, one with the changes to the tools tree, the
other with the changes to the sc tree.

Please review and apply if ok
Regards
Sören Möller
(LGPLv3+ / MPL)

2011/1/10 Kohei Yoshida <kyoshida@novell.com>:
> On Sun, 2011-01-09 at 21:41 +0000, Caolán McNamara wrote:
> > On Sun, 2011-01-09 at 20:37 +0100, Soeren Moeller wrote:
> > > And here another patch for sc/inc/funcdesc.hxx and related source
> > > files,
> > ...
> > > (using casts when forced to use String originally because of ResId).
> >
> > Kohei will probably want to have a look at these, but as an aside, how
> > about adding something like
> >
> > static rtl::OUString toString(const ResId&)
> >
> > to the tools resid.hxx ResId class and hide the nasty convert cast
> > kludge in the impl of that, which could be tucked away in the old
> > tools/source/strucvt.cxx for now.
>
> Yeah, that would make the transition a bit smoother.  We get String
> instances from ResId in many places, and having a function like that
> would indeed hide the ugliness in one place.
>
> The alternative is to use String to create a temporary string instance
> from (Sc)ResId, then pass it on to the constructor of rtl::OUString.
> But that always adds the overhead of instantiating two string instances
> instead of one.  Even with that overhead; however, I prefer that over
> casting String to OUString as long as the code is not
> performance-critical.  But Caolan's suggestion would probably be the
> best approach given the current constraints surrounding (Sc)ResId,
> String, and OUString.
>
> BTW, a minor nitpick to Soeren's original patch.  In GetParamList() and
> getSignature(), you could consider using rtl::OUStringBuffer instead of
> rtl::OUString for better performance.  rtl::OUString is by design
> immutable, and it creates a new string instance every time you add a new
> string segment to it.  The String class OTOH is not immutable hence it
> is sometimes used as a string buffer.  Looking at the way it is used in
> GetParamList() and getSignature(), rtl::OUStringBuffer probably is a
> better replacement candidate.
>
> Kohei
>
> --
> Kohei Yoshida, LibreOffice hacker, Calc
> <kyoshida@novell.com>
From 3b18cb9327a5706be0a2b968b9bc6773f0a0299d Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?S=C3=B6ren=20M=C3=B6ller?= <soerenmoeller2001@gmail.com>
Date: Tue, 11 Jan 2011 00:16:19 +0100
Subject: [PATCH] Added function to ResId to give an OUString from a ResId

---
 tools/inc/tools/resid.hxx |    8 ++++++++
 1 files changed, 8 insertions(+), 0 deletions(-)

diff --git a/tools/inc/tools/resid.hxx b/tools/inc/tools/resid.hxx
index 2b5cb21..b00bb62 100644
--- a/tools/inc/tools/resid.hxx
+++ b/tools/inc/tools/resid.hxx
@@ -30,7 +30,9 @@
 #define _TOOLS_RESID_HXX
 
 #include <tools/solar.h>
+#include <tools/string.hxx>
 #include <osl/diagnose.h>
+#include <rtl/ustring.hxx>
 
 struct RSHEADER_TYPE;
 typedef sal_uInt32 RESOURCE_TYPE;
@@ -158,6 +160,12 @@ class ResId
     
     sal_uInt32    GetId()                      const { return m_nResId & ~RSC_DONTRELEASE; }
     RSHEADER_TYPE* GetpResource()      const { return m_pResource; }
+
+    /* Gets a OUString by ResId via String, header in resid.hxx */
+    static ::rtl::OUString* toString(const ResId& resid)
+    {
+        return new ::rtl::OUString((::rtl::OUString)*(new String(resid)));
+    }
 };
 
 #endif // _RESID_HXX
-- 
1.7.0.4

From 36a1c045bd67403b7f19903e7f7844a6a8f03e7e Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?S=C3=B6ren=20M=C3=B6ller?= <soerenmoeller2001@gmail.com>
Date: Tue, 11 Jan 2011 00:21:51 +0100
Subject: [PATCH] OUString to OUStringBuffer and fixed ResId to OUString workaround

Replaced OUString with OUStringBuffer in two plaeces where a lot of appending is done, and replaced 
a workaround for getting a OUString from a ResId by calls to ResId::toString
---
 sc/source/core/data/global.cxx |   60 ++++++++++++++++++++--------------------
 1 files changed, 30 insertions(+), 30 deletions(-)

diff --git a/sc/source/core/data/global.cxx b/sc/source/core/data/global.cxx
index b3de8ba..cc991d9 100644
--- a/sc/source/core/data/global.cxx
+++ b/sc/source/core/data/global.cxx
@@ -1169,7 +1169,7 @@ ScFuncRes::ScFuncRes( ResId &aRes, ScFuncDesc* pDesc, bool & rbSuppressed )
     }
 
     pDesc->pFuncName = new ::rtl::OUString( ScCompiler::GetNativeSymbol( static_cast<OpCode>( 
aRes.GetId())));
-    pDesc->pFuncDesc = (::rtl::OUString*)(new String(ScResId(1)));
+    pDesc->pFuncDesc = ResId::toString(ScResId(1));
 
     if (nArgs)
     {
@@ -1177,8 +1177,8 @@ ScFuncRes::ScFuncRes( ResId &aRes, ScFuncDesc* pDesc, bool & rbSuppressed )
         pDesc->ppDefArgDescs = new ::rtl::OUString*[nArgs];
         for (USHORT i = 0; i < nArgs; i++)
         {
-            pDesc->ppDefArgNames[i] = (::rtl::OUString*)(new String(ScResId(2*(i+1)  )));
-            pDesc->ppDefArgDescs[i] = (::rtl::OUString*)(new String(ScResId(2*(i+1)+1)));
+            pDesc->ppDefArgNames[i] = ResId::toString(ScResId(2*(i+1)  ));
+            pDesc->ppDefArgDescs[i] = ResId::toString(ScResId(2*(i+1)+1));
         }
     }
 
@@ -1459,9 +1459,9 @@ void ScFuncDesc::Clear()
 
 ::rtl::OUString ScFuncDesc::GetParamList() const
 {
-    const String& sep = ScCompiler::GetNativeSymbol(ocSep);
+    ::rtl::OUString sep(ScCompiler::GetNativeSymbol(ocSep));
 
-    ::rtl::OUString aSig;
+    ::rtl::OUStringBuffer aSig;
 
     if ( nArgCount > 0 )
     {
@@ -1476,11 +1476,11 @@ void ScFuncDesc::Clear()
                 else
                 {
                     nLastAdded = i;
-                    aSig += *(ppDefArgNames[i]);
+                    aSig.append(*(ppDefArgNames[i]));
                     if ( i != nArgCount-1 )
                     {
-                        aSig += ::rtl::OUString(sep);
-                        aSig += ::rtl::OUString::createFromAscii( " " );
+                        aSig.append(sep);
+                        aSig.appendAscii( " " );
                     }
                 }
             }
@@ -1488,7 +1488,7 @@ void ScFuncDesc::Clear()
             // remove one "; "
             if (nLastSuppressed < nArgCount && nLastAdded < nLastSuppressed &&
                     aSig.getLength() >= 2)
-                aSig = aSig.copy(0,aSig.getLength() - 2);
+                aSig.setLength(aSig.getLength() - 2);
         }
         else
         {
@@ -1497,52 +1497,52 @@ void ScFuncDesc::Clear()
             {
                 if (!pDefArgFlags[nArg].bSuppress)
                 {
-                    aSig += *(ppDefArgNames[nArg]);
-                    aSig += ::rtl::OUString(sep);
-                    aSig += ::rtl::OUString::createFromAscii( " " );
+                    aSig.append(*(ppDefArgNames[nArg]));
+                    aSig.append(sep);
+                    aSig.appendAscii( " " );
                 }
             }
             /* NOTE: Currently there are no suppressed var args parameters. If
              * there were, we'd have to cope with it here and above for the fix
              * parameters. For now parameters are always added, so no special
              * treatment of a trailing "; " necessary. */
-            aSig += *(ppDefArgNames[nFix]);
-            aSig += ::rtl::OUString('1');
-            aSig += ::rtl::OUString(sep);
-            aSig += ::rtl::OUString::createFromAscii( " " );
-            aSig += *(ppDefArgNames[nFix]);
-            aSig += ::rtl::OUString('2');
-            aSig += ::rtl::OUString(sep);
-            aSig += ::rtl::OUString::createFromAscii(" ... " );
+            aSig.append(*(ppDefArgNames[nFix]));
+            aSig.appendAscii("1");
+            aSig.append(sep);
+            aSig.appendAscii( " " );
+            aSig.append(*(ppDefArgNames[nFix]));
+            aSig.appendAscii("2");
+            aSig.append(sep);
+            aSig.appendAscii(" ... " );
         }
     }
 
-    return aSig;
+    return aSig.makeStringAndClear();
 }
 
 //------------------------------------------------------------------------
 
 ::rtl::OUString ScFuncDesc::getSignature() const
 {
-    ::rtl::OUString aSig;
+    ::rtl::OUStringBuffer aSig;
 
     if(pFuncName)
     {
-        aSig = *pFuncName;
+        aSig.append(*pFuncName);
 
         ::rtl::OUString aParamList = GetParamList();
         if( aParamList.getLength() )
         {
-            aSig += ::rtl::OUString::createFromAscii( "( " );
-            aSig += aParamList;
+            aSig.appendAscii( "( " );
+            aSig.append(aParamList);
             // U+00A0 (NBSP) prevents automatic line break
-            aSig += ::rtl::OUString( static_cast< sal_Unicode >(0xA0) );
-            aSig += ::rtl::OUString( ')' );
+            aSig.append( static_cast< sal_Unicode >(0xA0) );
+            aSig.appendAscii( ")" );
         }
         else
-            aSig += ::rtl::OUString::createFromAscii( "()" );
+            aSig.appendAscii( "()" );
     }
-    return aSig;
+    return aSig.makeStringAndClear();
 }
 
 //------------------------------------------------------------------------
@@ -1851,7 +1851,7 @@ void ScFunctionMgr::fillLastRecentlyUsedFunctions(::std::vector< const 
formula::
     }
 
     ::std::auto_ptr<ScResourcePublisher> pCategories( new ScResourcePublisher( ScResId( 
RID_FUNCTION_CATEGORIES ) ) );
-    return (::rtl::OUString)String(ScResId((USHORT)_nCategoryNumber));
+    return *ResId::toString(ScResId((USHORT)_nCategoryNumber));
 }
 sal_Unicode ScFunctionMgr::getSingleToken(const formula::IFunctionManager::EToken _eToken) const
 {
-- 
1.7.0.4