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


From Eike Rathke <erack@redhat.com>:

Eike Rathke has posted comments on this change.

Change subject: Replaced deprecated tools/String with OUString in ScAddInCol
......................................................................


Patch Set 1: I would prefer that you didn't submit this

(15 inline comments)

Please do the changes lined out, thanks.
Sorry for nitpicking ;-)

....................................................
File sc/source/core/tool/addincol.cxx
Line 462:             aFuncName += ::rtl::OUString( pFuncNameArray[nFuncPos] );
Just a hint: these operator+= lines create temporary OUString instances (actually two per statement 
in this case) de/allocating memory that could be avoided by using an OUStringBuffer of a sufficient 
initial size (i.e. length of aServiceName plus '.' plus length of pFuncNameArray[nFuncPos]) and use 
OUStringBuffer::append() to concatenate and a final OUStringBuffer::makeStringAndClear() call to 
create the OUString. However, I think it doesn't matter that much here, just to make you aware of 
how that could be done.

Line 643:     xub_StrLen nPos = aFullName.lastIndexOf( (sal_Unicode) '.' );
The xub_StrLen (defined to sal_uInt16) is wrong now, OUString positions such as returned by 
lastIndexOf() are sal_Int32 instead.

Line 644:     if ( nPos != STRING_NOTFOUND && nPos > 0 )
So that means to also change comparisons with STRING_NOTFOUND (which is defined to 0xFFFF), 
lastIndexOf() returns -1 if not found, so the condition here can be simplified to

    if (nPos > 0)

Line 934:                                             aLocalU = rtl::OUString("###");
This should not need another temporary OUString anymore (not related to your changes, proper 
operator=() have been implemented meanwhile) and instead simply be
 aLocalU = "###";

Line 946:                                             aDescU = rtl::OUString("###");
Same here,
 aDescU = "###";

Line 948:                                         ::rtl::OUString aDescription = ::rtl::OUString( 
aDescU );
This can simply be
 ::rtl::OUString aDescription( aDescU );

Line 971:                                                         aArgName = rtl::OUString("###");
aArgName = "###";

Line 981:                                                         aArgName = rtl::OUString("###");
aArgName = "###";

Line 990:                                                     aDesc.aDescription = ::rtl::OUString( 
aArgDesc );
Temporary conversion ctors are unnecessary here now, so
 aDesc.aName = aArgName;
 aDesc.aDescription = aArgDesc;

Line 1165:                                                 aDesc.aName = aDesc.aDescription = 
::rtl::OUString::createFromAscii( "###" );
aDesc.aName = aDesc.aDescription = "###";

Line 1319:     if (!aDesc.getLength())
For this !...getLength() we now have isEmpty(), so
 if (aDesc.isEmpty())

Line 1609:                 aString = ::rtl::OUString( aUStr );
As the aString member variable now is of type OUString the temporary isn't needed anymore, so this

 rtl::OUString aUStr;
 rNewRes >>= aUStr;
 aString = ::rtl::OUString( aUStr );

instead can simply be

 rNewRes >>= aString;

....................................................
File sc/source/core/tool/compiler.cxx
Line 5199:     rName = String(prName);
In our convention a leading small p indicates a pointer, this should be aName instead of prName. 
There's also a converting String::operator=(const OUString&) so this temporary String can be 
simplified to

 ::rtl::OUString aName(rName);
 ScGlobal::GetAddInCollection()->LocalizeString( aName );
 rName = aName;

Line 5245:                     aEntry.Name = String(aName);
Same here,

 aEntry.Name = aName;

is sufficient, with the additional benefit that once we also converted aEntry.Name to OUString 
there wouldn't be a superfluous automatic back and forth conversion.

....................................................
File sc/source/filter/excel/xiroot.cxx
Line 282:         return String(aScName);
Using the automatic conversion should work here

 return aScName;

--
To view, visit https://gerrit.libreoffice.org/258
To unsubscribe, visit https://gerrit.libreoffice.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I7059f10617b9a33ba63690c980b96d95d9023c55
Gerrit-PatchSet: 1
Gerrit-Project: core
Gerrit-Branch: master
Gerrit-Owner: Sören Möller <soerenmoeller2001@gmail.com>
Gerrit-Reviewer: Eike Rathke <erack@redhat.com>


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.