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

On 15/12/10 10:04, Noel Power wrote:
> 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
pushed to master, hmmm I noticed running the test document the results 
are slightly worse than previously, I need to investigate how I seem to 
have broken this. I was sure that I retested ( all but the last changes 
I made ) which I didn't think would affect the results adversely. 
Anyway, not to worry I will fix that.

Noel