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

Hi John,
On 06/12/10 17:58, Noel Power wrote:
>  Hi John,
[...]
> 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
I removed the '#if MAYBEFUTURE'  I put around the code above. After some 
poking around and also looking at the dev guide it seems this code is to 
handle the case where the expected type ( on the uno side ) is unknown, 
in this case the value is down cast to the smallest ( non-internal ) 
basic type which is then converted to an uno type ( or at least that's 
my current take on the intention )
> 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'm not sure it is needed but I enabled this ( and created a new stream 
operator to handle the 64 bit type )
> 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 ).
I also rewrote the ImpStringToCurrency a little, actually more like I 
combined yours and the old implementation a little, mostly I just 
removed the home grown parser in favour of using the existing 
string->number conversions in OUString. Like the ImpCurrencyToString I 
have enclosed the locale specific functionality in a '#if MAYBEFUTURE' 
block. I removed the acceptance of blank characters in the middle of a 
string as VBA regards this as an error and the old implementation just 
truncated the number ( e.g. "12345 6" would end up as 12345 ), I raise 
an error in this case now. Hopefully if this breaks existing macros it 
will only break code already deeply in error ( from mis-interpreting the 
string/number  )  The other reason for removing the parser is if/when we 
do apply some locale specific separator processing I would love to 
reuse/share whatever calc currently uses.
> 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 still haven't figured out if this code is really used or not, 
currently erring on the side of caution I created a new stream operator 
for SvStream and additionally tweaked the handling for Currency and the 
SbxSALINT64 and SbxSALUINT64 from you orig patch, it seems there were 
some bugs with the orig patch. Currently I don't see an easy way of 
testing the changes ( so I have coded them untested ) so I hope that 
maybe another set of eyes might help. Please feel free to do that ( and 
I'll try and ask on the list for some help too )
> 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?
did you look at the changes I committed to the branch ? ( of course 
there are some more changes to look at now :-) ) I'd be interested in 
your thoughts as I would like to get this stuff into master asap

Noel