Re: [Libreoffice] [PATCH] Basic Currency Issues #i31001# to #i107277#

 Hi John,

On 30/11/10 09:35, John LeMoyne Castle wrote:
> Currency is presently implemented with a 64bit struct of 2 Int32s and the
> math is done by the tools/bigint class.
> The attached patches replace the code to implement the Currency type with an
> ISO C99 compiler supported 64 bit integer.
Absolutely, using 64bit integer types makes sense
> In fact, the 64 bit int was already a member of the Sbx core data union.
[...]
> http://nabble.documentfoundation.org/file/n1991648/0001-Basic-Currency-64bit-impl-fixes-i31001-to--libs-core.patch.tar.gz
> 0001-Basic-Currency-64bit-impl-fixes-i31001-to--libs-core.patch.tar.gz
This is integrated now in the feature branch  feature/currency-64bit, in 
the main I am really happy with this patch, here is a summary of things 
I changed/modified etc.

First off the patch didn't build for me, at a wild guess I'd say maybe 
you built this on a 32 bit system ( as the error seemed 64 bit related.. 
but I am not completely sure :-) ) error was..

 ../../inc/basic/sbxvar.hxx:92:5: error: 
'SbxValues::SbxValues(sal_Int64)' cannot be overloaded
./../inc/basic/sbxvar.hxx:87:5: error: with 'SbxValues::SbxValues(long 
int)'
dmake:  Error code 1, while making '../../unxlngx6.pro/obj/sbintern.obj'

in anycase I removed the associated SbxValues constructor as it seemed 
to enforce SbxValues::SbxValues(sal_Int64) would set the type of 
SbxValues as SbxCURRENCY. I don't think it's a good idea to enforce 
that, sal_Int64 just happens to the the data type we use for the 
SbxCURRENCY type, it isn't indicative of the type itself.

I reverted the change below

-#define SbxMININT            (-32768)
+#define SbxMININT   (-32767)

I thought perhaps this wasn't intentional until I saw similar in the 
test document, did you have a specific reason for changing that, imo we 
can't change that as it is a hard limit. ( btw I changed it in both the 
test document and the code and see no difference in the generated results )

I removed various comments that looked I guess like internal notes to 
yourself, please have a look and ensure that I didn't remove something 
you really want to keep. As an example ( from basic/inc/basic/sbxdef.hxx )

-#define    SBX_MAXINDEX        0x3FF0
-#define    SBX_MAXINDEX32        SbxMAXLNG
+// ?can we remove this ~16k limit on array index/size?
+#define SBX_MAXINDEX    0x3FF0
+#define SBX_MAXINDEX32  SbxMAXLNG
+

as an aside of course Arrays and such are no longer limited ( afaik ) by 
16bit  I guess we probably could get rid of the many side by side 16 and 
32 bit variants of some method ( see Get32 & Get methods in SbxArray ) 
But that would be another task ( and I am not really sure the effort to 
do that is worth it with so many other problems to look at )

in  sbxToUnoValueImpl ( convert to uno without known target class ) in 
basic/source/classes/sbunoobj.cxx previously an Double or Float basic 
value ( within the range of an int ) was transfered as a the smallest 
type e.g. sal_Int8, sal_Int16 or sal_Int32 values greater were 
transferred as Float or Double
   * there is a bit possibility that we could break existing code, care 
needed here I thing ( transferring the value as sal_Int64 might seem 
natural but hyper seems to be rarely used in uno interfaces ) not sure 
about this, I am tempted to just let things break ( 'cause they seem 
wrong in the first place ) Otoh the comments in the code do suggest 
converting to int is intentional. ( erring on the side of caution it's 
#ifdefed out ( with #if MAYBEFUTURE ) at the moment )

I see you have wiped the following types SbxLONG64 & SbxULONG64 from the 
face of the earth ;-) I can't see that these are used anywhere and I 
would guess they are handled probably in the binary filter code only. So 
I hope this change is ok too.

in basic/source/runtime/methods1.cxx you have added incomplete 
read/write support for types SbxSALINT64 & SbxSALUINT64,  lets not add 
support to persist a new type incompletely ( to easy to forget about ) 
so I've  #ifdefed it out ( with #if IMPLEMENTATION_READY )

I fixed a couple of identical treatment of SALxINT64 & SbxCURRENCY types 
e.g. sbxbyte ( there were others too ) these break things like


dim a as currency
dim b as byte:w

a = 1
b = a '<- overflows here ( clearly incorrect )

other types are similarly affected

similar to the above there I also fixed some mismatch with the Get/Put ( 
maybe some of this was pre-existing, I wouldn't be suprised )

I re-added some checks for removed limit checks ( actually I hope that I 
didn't make things worse here as all this similar methods started making 
my head spin, it's quite possible I screwed something up )

changed signature of ImpPutCurrency( sal_Int64& n ) 'tis a bad idea 
'cause the incoming could be unexpectedly changed ( I realise this is an 
oversight due to the fact that previously this method had an object 
passed in the signature ) perhaps I am being paranoid... but anyway I 
changed it

fixed some missing breaks in various case statements, e.g. at there was 
a break removed from sbxdec ( within case SbxBYREF | SbxSINGLE ) was it 
a intentional? ( I added back in the break but maybe I was wrong )
and also in sbxlng SbxSALINT64, SbxSALUINT64 missing breaks ( I think 
there were some others too )

I rewrote the ImpCurrencyToString, I took note of your suggestions re. 
optimizations whilst trying to point you to some existing functionality 
available from the String classes as its better to avoid generating even 
more number to string code. Somewhere between trying to write down some 
explanations and suggestions I'm afraid I found myself actually 
rewriting this and then I found mistakes in my 'new' implementation... 
couldn't help myself. Anyway hopefully some of the code there shows some 
other possibilities to achieve the same thing reusing some of the 
existing LO classes. ( I have no doubt that there probably is some bugs 
in my implementation too ). Additionally I am still very uncertain about 
  ImpStringToCurrency() changes to use Locale specific separators. Also 
not sure about the local seperator logic, the heuristics used I think 
might give an undesired result under some circumstances. I don't want to 
rule it out but for the moment I would feel happier with the locale 
specific part being #ifdefed out ( so I did that with some more   #if 
MAYBEFUTURE blocks ).


SbxValue::LoadData & SbxValue::SaveData, we need to ensure that a 
Currency val written from the old Currency implementation can be read 
with the new implementation and vice-versa ( looking at the code it 
seems that it should work ). Hmmm but thinking about this more I really 
don't see why this is necessary, I can't think of why a types value 
would be saved with this this code, I wonder is it really dead code, 
note the Load/Save methods are definitely used ( one usage I know about 
is when saving password protected macros ) but... in this case it is the 
pcode that is saved and I can't see why/where a variables value would be 
saved. It's also possible the automation framework might use that ( but 
that shouldn't be a problem either). So a) need to confirm the 
read/write the old/new Currency type ( similarly the transfer of 
oleautomation::Currency ) b) would be great to investigate further to 
see what can be removed from those LoadData/SaveData methods.

It would be great if you could look over my changes and see if they are 
ok, if you are happy with the change and if we can satisfy ourselves 
that LoadData/SaveData item above is not an issue then we could probably 
commit this to the master, then we can look further into some of the 
ifdefed out stuff, how does that sound?

thanks again for the great patch and really cool test document

Noel
> http://nabble.documentfoundation.org/file/n1991648/0001-Basic-Currency-type-native-64bit-implemen-components.patch
> 0001-Basic-Currency-type-native-64bit-implemen-components.patch
>
> http://nabble.documentfoundation.org/file/n1991648/CurTestODS.ods.tar.gz
> CurTestODS.ods.tar.gz
>
> The attached CurTestODS.ods spreadsheet Basic program CurTestRunAll
> demonstrates the errors from the old implementation:
>    --  flagrantly wrong values from string inputs
>    --  hugely wrong values from MDAS calculation
> (where flagrantly, hugely == 10+ orders of magnitude off)
>    --  no-op (returning one of the operands instead of doing the operation)
>    --  incorrect overflow from operations on bad string input values
>    --  different errors and error rates on different platforms (in LibO Beta
> 3 on Ubuntu10.04 and WinVista)
>
> The errors appear to start when results reach ~2.15e5 [(2^31-1)/10,000].
> They continue up through the Currency upper limit of ~1e15.
> The errors affect all of the basic MDAS (*/+-) math operations mostly(?) add
> and subtract.
> I didn't troubleshoot the errors thoroughly because the offending code was
> either:
> ripped out and replaced with simple C++ operations on the compiler supported
> 64 bit int, or
> removed by rework of the Compute, Get and Put routines in sbx/*.
I think probably there was maybe a single point of failure here that 
rippled through, certainly
> The test program is CurTestODS.ods :: Standard :: CurTestRunAll
> CurTestRunAll tests number pairs from the CurTestMeta sheet by:
>    -2x-  reading the numbers in as strings or doubles
>    -4x-  performing all MDAS (*/+-) operations
>   -16x-  for all Basic operand type combinations of integer(16b), long (32b),
> double (64b) and currency
> for 128 tests on each number pair each test producing a Currency type
> result.
> Expected values are calculated by a parallel calculation in doubles to
> produce an expected Currency type result.
> Anticipated Basic run-time errors (overflow and div by zero) are identified
> and passed.
> Unexpected BRT errors are counted and output as errors.
>
> All of the PostFix result sheets show a great reduction in the number of
> errors over Beta3 when the new implementation is used.
>
> The 4 remaining "errors" in the Demo result set show both the strength and
> the weakness of the fixed point Currency type relative to the double
> precision floating point type.  These type differences pre-exist by
> definition and will not go away in any implementation.  Where more
> fractional digits are needed the Double is more accurate, but when more
> significant digits are needed the Currency type is more accurate.
>
> What else it does:
> The fix needed to include corrections to the
> currency-get-value-from-string-literal because it was a source of bad
> values.  Users have requested in OOo issues to get the ability to use string
> literals in their Locale format as set in the options.  The new string
> getter relaxes the requirement that string literal be in en-US format (i.e.
> "1,000,000.00") - it will now accept "1.000.000,00" if the Locale allows it
> while always accepting the Basic standard en-US format.  The getter will
> also always accept (always ignore) spaces within a number string so "1 000
> 000.00 00" is accepted.  The Locale read was lifted from another of the
> Currency string input functions.  If this needs rework or reversion let me
> know.
>
> I thought this would be easy - and it was easy and fun - it was just a real
> Big Easy...
>
> All contributions licensed per TDF standard:  MPL 1.1 / GPLv3+ / LGPLv3+
>
> --LeMoyne (JLCastle)