Change in core[master]: fdo#57180 add calc function NUMBERVALUE as defined in ODFF1....

[Eike Rathke (via Code Review)]

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 <osc at dci-electronics.nl>
Gerrit-Reviewer: Eike Rathke <erack at redhat.com>
Gerrit-Reviewer: Winfried Donkers <osc at dci-electronics.nl>