Re: [Libreoffice] [PATCH] Replace (Byte)String with O(U)String
On Tue, 2012-01-17 at 11:06 +0100, Chr. Rossmanith wrote:
Hi,
an excursion from vcl to unotools was necessary. Could someone please
review this little patch? xub_StrLen is replaced as well with sal_Int32.
You'll have to change the callers of RecodeString
(http://opengrok.libreoffice.org/xref/core/vcl/source/gdi/outdev3.cxx#6009) to
use an rtl::OUString as well.
On this patch in isolation, you could make one stringbuffer before the
loop, and set the returned string from that at the end, and just use one
stringbuffer in the loop, something like...
rtl::OUStringBuffer aTmpStr(rStr);
for(; nIndex nLastIndex; ++nIndex )
{
sal_Unicode cOrig = aTmpStr[nIndex];
...
if( cOrig != cNew )
aTmpStr[nIndex] = cNew;
...
}
rStr = aTmpStr.makeStringAndClear();
C.
___
LibreOffice mailing list
LibreOffice@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/libreoffice
Re: [Libreoffice] [PATCH] Replace (Byte)String with O(U)String
On 01/17/2012 11:06 AM, Chr. Rossmanith wrote:
an excursion from vcl to unotools was necessary. Could someone please
review this little patch? xub_StrLen is replaced as well with sal_Int32.
Two things about the patch:
-void ConvertChar::RecodeString( String rStr, xub_StrLen nIndex, xub_StrLen
nLen ) const
+void ConvertChar::RecodeString( rtl::OUString rStr, sal_Int32 nIndex,
sal_Int32 nLen ) const
{
-sal_uLong nLastIndex = (sal_uLong)nIndex + nLen;
-if( nLastIndex rStr.Len() )
-nLastIndex = rStr.Len();
+sal_Int32 nLastIndex = nIndex + nLen;
+if( nLastIndex rStr.getLength() )
+nLastIndex = rStr.getLength();
The original nIndex, nLen were 16 bit, so their sum was guaranteed to
fit into 32 bit sal_uLong. There is no such guarantee with the new
nIndex, nLen of 32 bit. But looking around, the only call of
RecodeString is
mpFontEntry-mpConversion-RecodeString( aStr, 0, aStr.Len() );
in vcl/source/gdi/outdev3.cxx. So this can all go away, changing the
signature of RecodeString to
rtl::OUString RecodeString(rtl::OUString const )
and adapting the single call side accordingly (also getting rid of the
in-out parameter).
for(; nIndex nLastIndex; ++nIndex )
{
-sal_Unicode cOrig = rStr.GetChar( nIndex );
+sal_Unicode cOrig = rStr[nIndex];
// only recode symbols and their U+00xx aliases
if( ((cOrig 0x0020) || (cOrig 0x00FF))
((cOrig 0xF020) || (cOrig 0xF0FF)) )
@@ -1390,8 +1391,12 @@ void ConvertChar::RecodeString( String rStr, xub_StrLen
nIndex, xub_StrLen nLen
// recode a symbol
sal_Unicode cNew = RecodeChar( cOrig );
-if( cOrig != cNew )
-rStr.SetChar( nIndex, cNew );
+if( cOrig != cNew ) {
+//rStr[nIndex] = cNew;
+rtl::OUStringBuffer aTmpStr(rStr.getStr());
+aTmpStr[nIndex] = cNew;
+rStr = aTmpStr.makeStringAndClear();
+}
Constructing a temporary OUStringBuffer for each character that needs
conversion smells somewhat inefficient. An alternative is to put the
original rStr into an OUStringBuffer once and iterate over that one.
Stephan
___
LibreOffice mailing list
LibreOffice@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/libreoffice
Re: [Libreoffice] [PATCH] Replace (Byte)String with O(U)String
Hi Caolan and Stephan,
Am 17.01.2012 13:40, schrieb Caolán McNamara:
On Tue, 2012-01-17 at 11:06 +0100, Chr. Rossmanith wrote:
an excursion from vcl to unotools was necessary. Could someone please
review this little patch? xub_StrLen is replaced as well with sal_Int32.
You'll have to change the callers of RecodeString
(http://opengrok.libreoffice.org/xref/core/vcl/source/gdi/outdev3.cxx#6009) to
use an rtl::OUString as well.
That is where my excursion started :-) i.e. the reason why I had to
modify RecodeString()
On this patch in isolation, you could make one stringbuffer before the
loop, and set the returned string from that at the end, and just use one
stringbuffer in the loop, something like...
rtl::OUStringBuffer aTmpStr(rStr);
for(; nIndex nLastIndex; ++nIndex )
{
sal_Unicode cOrig = aTmpStr[nIndex];
...
if( cOrig != cNew )
aTmpStr[nIndex] = cNew;
...
}
rStr = aTmpStr.makeStringAndClear();
I'll add this to my patch and Stephan Bergmanns improvements as well.
Thank you,
Christina
___
LibreOffice mailing list
LibreOffice@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/libreoffice
Re: [Libreoffice] [PATCH] Replace (Byte)String with O(U)String
Hi,
this is a modified version of the previous patch which takes into
account improvements suggested by Caolan and Stephan. Please don't push
yet because the caller of RecodeString() has to be touched as well. Does
that modification have to be contained in the same patch or do I just
have to make sure that the two commits are pushed together? A second
review is very welcome.
Christina
From ac6a68b5bc875880c1d9d5f2cd7650302ef4dcad Mon Sep 17 00:00:00 2001
From: Christina Rossmanith chrrossman...@web.de
Date: Tue, 17 Jan 2012 11:02:34 +0100
Subject: [PATCH] Replace (Byte)String with O(U)String / simplify ConvertChar::RecodeString()
---
unotools/inc/unotools/fontdefs.hxx |2 +-
unotools/source/misc/fontcvt.cxx | 16
2 files changed, 9 insertions(+), 9 deletions(-)
diff --git a/unotools/inc/unotools/fontdefs.hxx b/unotools/inc/unotools/fontdefs.hxx
index 12d9c43..ac02320 100644
--- a/unotools/inc/unotools/fontdefs.hxx
+++ b/unotools/inc/unotools/fontdefs.hxx
@@ -74,7 +74,7 @@ public:
const char* mpSubsFontName;
sal_Unicode (*mpCvtFunc)( sal_Unicode );
sal_Unicode RecodeChar( sal_Unicode c ) const;
-voidRecodeString( String rStra, xub_StrLen nIndex, xub_StrLen nLen ) const;
+voidRecodeString( rtl::OUString rStra ) const;
static const ConvertChar* GetRecodeData( const String rOrgFontName, const String rMapFontName );
};
diff --git a/unotools/source/misc/fontcvt.cxx b/unotools/source/misc/fontcvt.cxx
index cabdaf7..d81f725 100644
--- a/unotools/source/misc/fontcvt.cxx
+++ b/unotools/source/misc/fontcvt.cxx
@@ -29,6 +29,7 @@
#include unotools/fontcvt.hxx
#include unotools/fontdefs.hxx
#include sal/macros.h
+#include rtl/ustrbuf.hxx
#ifndef _STLP_MAP
#include map
@@ -1374,15 +1375,12 @@ sal_Unicode ConvertChar::RecodeChar( sal_Unicode cChar ) const
// recode the string assuming the character codes are symbol codes
// from an traditional symbol font (i.e. U+F020..U+F0FF)
-void ConvertChar::RecodeString( String rStr, xub_StrLen nIndex, xub_StrLen nLen ) const
+void ConvertChar::RecodeString( rtl::OUString rStr ) const
{
-sal_uLong nLastIndex = (sal_uLong)nIndex + nLen;
-if( nLastIndex rStr.Len() )
-nLastIndex = rStr.Len();
-
-for(; nIndex nLastIndex; ++nIndex )
+rtl::OUStringBuffer aTmpStr(rStr.getStr());
+for(sal_Int32 nIndex=0; nIndex rStr.getLength(); ++nIndex )
{
-sal_Unicode cOrig = rStr.GetChar( nIndex );
+sal_Unicode cOrig = aTmpStr[nIndex];
// only recode symbols and their U+00xx aliases
if( ((cOrig 0x0020) || (cOrig 0x00FF))
((cOrig 0xF020) || (cOrig 0xF0FF)) )
@@ -1391,8 +1389,10 @@ void ConvertChar::RecodeString( String rStr, xub_StrLen nIndex, xub_StrLen nLen
// recode a symbol
sal_Unicode cNew = RecodeChar( cOrig );
if( cOrig != cNew )
-rStr.SetChar( nIndex, cNew );
+aTmpStr[nIndex] = cNew;
+
}
+rStr = aTmpStr.makeStringAndClear();
}
//===
--
1.7.4.1
___
LibreOffice mailing list
LibreOffice@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/libreoffice
Re: [Libreoffice] [PATCH] Replace (Byte)String with O(U)String
On 01/17/2012 09:45 PM, Chr. Rossmanith wrote: this is a modified version of the previous patch which takes into account improvements suggested by Caolan and Stephan. Please don't push yet because the caller of RecodeString() has to be touched as well. Does that modification have to be contained in the same patch or do I just have to make sure that the two commits are pushed together? A second review is very welcome. It is best to do it as a single patch (doing it in two steps would produce problems for git bisect, for example). The patch looks good now (only nit to pick is to consider moving from an in--out parameter to an in-only parameter and rtl::OUString return type; out parameters, esp. if they hide behind a reference type, can be confusing -- think about somebody trying to grok where a given variable aStr can change its value, and overlooking something innocuous-looking like mpFontEntry-mpConversion-RecodeString(aStr); where aStr = mpFontEntry-mpConversion-RecodeString(aStr); would be so much clearer). ___ LibreOffice mailing list LibreOffice@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/libreoffice
Re: [Libreoffice] [PATCH] Replace (Byte)String with O(U)String in sw
On 10/03/2011 04:48 PM, Chr. Rossmanith wrote: One improvement suggested by Norbert on IRC: using OUString instead of ::rtl::OUString. What is the preferred syntax using ::rtl::OUString at the beginning of the file and OUString throughout the remaining source code or ::rtl::OUString everywhere in the source code. I would stick with a pattern that does use a namespace prefix (i.e., rtl::OUString rather than OUString). Even if it is slightly longer, it helps avoid ambiguities should one of the #included headers in the future drag in another entity with the same plain name (probably unlikely for OUString, but it does happen, cf. rtl::Reference vs. com::sun::star::uno::Reference; another measure against this is to explicitly using rtl::OUString; instead of the more general using namespace rtl;). The initial :: in ::rtl::OUString is probably not really worth it, more noise than of practical value. But there's a lot of personal taste involved... -Stephan ___ LibreOffice mailing list LibreOffice@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/libreoffice
Re: [Libreoffice] [PATCH] Replace (Byte)String with O(U)String in sw
Hi Christina, On Sun, 2011-10-02 at 20:52 +0200, Chr. Rossmanith wrote: could someone please review this small patch. I'll extend the replacement (and push on my own) but want to be sure that there are no mistakes. At least it seems to build :-) Looks great to me :-) Thanks ! Michael. -- michael.me...@suse.com , Pseudo Engineer, itinerant idiot ___ LibreOffice mailing list LibreOffice@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/libreoffice
Re: [Libreoffice] [PATCH] Replace (Byte)String with O(U)String in sw
On Mon, 2011-10-03 at 10:07 +0100, Michael Meeks wrote: Hi Christina, On Sun, 2011-10-02 at 20:52 +0200, Chr. Rossmanith wrote: could someone please review this small patch. I'll extend the replacement (and push on my own) but want to be sure that there are no mistakes. At least it seems to build :-) Looks great to me :-) yup, fine, no errors there. FWIW, personally, I've tended towards... rtl::OStringBuffer aError(RTL_CONSTASCII_STRINGPARAM(foo)); aError.append(::rtl::OUStringToOString(r.Message, osl_getThreadTextEncoding()); OSL_FAIL(aError.getStr()); But its just an OSL_FAIL, so whatever titchy (if any) difference using a OStringBuffer and RTL_CONSTASCII_STRINGPARAM makes is irrelevant there, and the same goes for the output encoding really, its going to be ascii, so ASCII, UTF-8, system encoding for the error message will all be equivalent in the real world. C. ___ LibreOffice mailing list LibreOffice@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/libreoffice
Re: [Libreoffice] [PATCH] Replace (Byte)String with O(U)String in sw
One improvement suggested by Norbert on IRC: using OUString instead of ::rtl::OUString. What is the preferred syntax using ::rtl::OUString at the beginning of the file and OUString throughout the remaining source code or ::rtl::OUString everywhere in the source code. Christina Am 03.10.2011 11:07, schrieb Michael Meeks: Hi Christina, On Sun, 2011-10-02 at 20:52 +0200, Chr. Rossmanith wrote: could someone please review this small patch. I'll extend the replacement (and push on my own) but want to be sure that there are no mistakes. At least it seems to build :-) Looks great to me :-) Thanks ! Michael. ___ LibreOffice mailing list LibreOffice@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/libreoffice
Re: [Libreoffice] [PATCH] Replace (Byte)String with O(U)String in sw
On Mon, Oct 3, 2011 at 9:48 AM, Chr. Rossmanith chrrossman...@gmx.de wrote: One improvement suggested by Norbert on IRC: using OUString instead of ::rtl::OUString. What is the preferred syntax Note: I don't know what is the 'preferred' way, I just noticed that there was a mix of the 2 notations (actually 3 even ::rtl::Foo, rtl::Foo and Foo) in the same function... which ever way is picked, I think that way should be used homogeneously in a given source. Norbert ___ LibreOffice mailing list LibreOffice@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/libreoffice
