Hi Daniel, On Tuesday, 2008-10-21 09:41:05 +0200, Daniel Rentz wrote:
> +BOOL lclConvertMoney( const String aSearchUnit, double& rfRate, int& > rnDec ) Btw, aSearchUnit should be a const reference instead of a copy, so const String & aSearchUnit ^^^ > +{ > +#define COUNT 16 > +#define CURRENCY( name ) \ > + (String::CreateFromAscii( RTL_CONSTASCII_STRINGPARAM( name ) )) > + ConvertInfo aConvertTable[ COUNT ] = { > + { CURRENCY( "EUR" ), 1.0, 2 }, > > > I am not sure (->Eike?) but initializing the strings this way may fail > with certain compilers. Yes, initializing static instances of class objects isn't guaranteed to always work in the order assumed (well, it probably would in this case), plus it adds unnecesary overhead when initializing the library's data. One more thing: the often observed pattern String aFoo( String::CreateFromAscii( RTL_CONSTASCII_STRINGPARAM( "literal" ))) is better written as String aFoo( RTL_CONSTASCII_USTRINGPARAM( "literal" )) with the advantage that an approriate ctor is called that already includes the count of characters and no temporary instance of class String is needed just to pass to the final String copy-ctor. Note the U in USTRINGPARAM, without that you would get a different parameter count and meaning! However, the latest revision of the patch already changed the table layout to use ASCII literals. The String::CreateFromAscii(...) still applies though. > Please do not #define the COUNT but calculate the array size after the > array. In the future, more countries may convert to the Euro currency. > This may be the source of errors when the next developer forgets to > update the COUNT after adding a line to the list. Which (w|sh)ould make the compiler complain about extra elements ... anyway > Better: > > const ConvertInfo aConvertTable[] = { > ... > }; > > const int COUNT = sizeof( aConvertTable ) / sizeof( ConvertInfo ); Some nitpicks: - Counts shouldn't be of type int but size_t instead. - COUNT is a very common name that may clash sooner or later with some includes, I suggest nConversionCount or such. - Instead of sizeof(ConvertInfo) use sizeof(aConvertTable[0]), this way one would not had to adapt that place in case the name of ConvertInfo would have to be changed for whatever reason. > if( maXclUrl.EqualsIgnoreCaseAscii( "\008EUROTOOL.XLA" ) ) > > Note the characters \008 which encode the ASCII character 8. Erm.. \010, just for the records, but we already clarified that on IRC ;-) As a side note maybe interesting to others reading this: a literal "\x08EUROTOOL.XLA" failed because the compiler takes \x08E as one entity and thus produces the string 0x8E,"UROTOOL.XLA" ... Eike -- OOo/SO Calc core developer. Number formatter stricken i18n transpositionizer. SunSign 0x87F8D412 : 2F58 5236 DB02 F335 8304 7D6C 65C9 F9B5 87F8 D412 OpenOffice.org Engineering at Sun: http://blogs.sun.com/GullFOSS Please don't send personal mail to the [EMAIL PROTECTED] account, which I use for mailing lists only and don't read from outside Sun. Use [EMAIL PROTECTED] Thanks.
pgplSq7Kq8DN5.pgp
Description: PGP signature