From Eike Rathke <er...@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 <soerenmoeller2...@gmail.com>
Gerrit-Reviewer: Eike Rathke <er...@redhat.com>
_______________________________________________
LibreOffice mailing list
LibreOffice@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/libreoffice
