Eike Rathke has posted comments on this change.
Change subject: fdo#57180 add calc function NUMBERVALUE as defined in ODFF1.2
......................................................................
Patch Set 3: (10 inline comments)
We're nearing the goal :-)
Please see my inline comments.
If you want me to take over just tell me, otherwise please submit a new patch
set for this change.
....................................................
File sc/source/core/tool/interpr1.cxx
Line 3392: sal_Unicode cDecimalSeparator;
cDecimalSeparator needs to be initialized with 0 here, so comparison
if(cDecimalSeparator&&...) below works.
Line 3423: {
Why only do these checks if there was nGlobalError?
Line 3434: }
Better would be to have
if (nGlobalError)
{
PushError( nGlobalError);
return;
}
if (aInputString.isEmpty())
{
if (GetGlobalConfig().mbEmptyStringAsZero)
PushDouble(0);
else
PushNoValue();
return;
}
Line 3442: }
Performance wise it is better to search for group separators in the string
because usually there's only one separator. Instead of replaceAt() use
replaceAll() that replaces all occurrences, for this just copy the string to a
temporary not including the decimal separator and the fractional part and
operate on the temporary copy, and when done concatenate the result with the
decimal separator plus fractional part again. Also,
aGroupSeparator.iterateCodePoints() should be used to obtain one separator at a
time and create the string to be searched, to allow for full UTF-16 characters,
like this:
sal_Int32 nDecSep = aInputString.indexOf( cDecimalSeparator);
if (nDecSep != 0)
{
OUString aTemporary( nDecSep >= 0 ? aInputString.copy( 0, nDecSep) :
aInputString);
sal_Int32 nIndex = 0;
do
{
sal_uInt32 nChar = aGroupSeparator.iterateCodePoints( nIndex);
aTemporary = aTemporary.replaceAll( OUString( &nChar, 1), "");
} while (nIndex < aGroupSeparator.getLength());
if (nDecSep >= 0)
aInputString = aTemporary + aInputString.copy( nDecSep);
else
aInputString = aTemporary;
}
All unchecked, may contain errors ;-)
Line 3451: for ( i = aInputString.getLength() - 1; aInputString[ i ] ==
0x0025 && i >= 0; i-- )
The condition needs to be swapped so it doesn't access [-1] memory
i >= 0 && aInputString[ i ] == 0x0025
Line 3453: if ( aInputString[ i ] == 0x0025 )
There is no need to check this again, the condition of the for loop already did
this.
Line 3462: double fVal = ::rtl::math::stringToDouble( aInputString,
cDecimalSeparator, 0, &eStatus, &nParseEnd );
Yes, correct. Sorry, my bad, I was confusing with the underlying
rtl_math_stringToDouble() function that takes the sal_int32**
....................................................
File sc/source/filter/excel/xlformula.cxx
Line 405: EXC_FUNCENTRY_ODF( ocNoName, 2, 2, 0, "IFNA" ),
Oops, this IFNA should not be here, probably due to your rebase.
Line 445
Do not remove this, please check your rebase.
....................................................
File sc/source/filter/oox/formulabase.cxx
Line 727: { "NUMBERVALUE", 0, NOID, NOID,
1, 3, V, { VR }, FUNCFLAG_MACROCALLODF }
Should be FUNCFLAG_MACROCALL instead of FUNCFLAG_MACROCALLODF and have the
"NUMBERVALUE" name instead of 0
However, with my latest change in commit
1162738c6fbd8505ffa27b28118318cc522a5368 that now should be
FUNCFLAG_MACROCALL_NEW instead.
--
To view, visit https://gerrit.libreoffice.org/1477
To unsubscribe, visit https://gerrit.libreoffice.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I6ee01764ae9fc27854fd3bd8a630b9d3560192e5
Gerrit-PatchSet: 3
Gerrit-Project: core
Gerrit-Branch: master
Gerrit-Owner: Winfried Donkers <[email protected]>
Gerrit-Reviewer: Eike Rathke <[email protected]>
Gerrit-Reviewer: Winfried Donkers <[email protected]>
_______________________________________________
LibreOffice mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/libreoffice
