Re: [Libreoffice] [PATCH] Add numbering type in persian word.

Hi Mohammad,

On Saturday, 2011-08-27 04:10:40 +0430, Mohammad Elahi wrote:

> Numbering in localized persian word is useful in some areas, at least
> for me it is necessary. Since it is not trivial a function and table
> is needed.

Good one, I'll also integrated this. I added quite a few spaces though
between variables and operators and keywords for better legibility.
I also changed some of the implementation, please see my comments on
below, for the results please see the commit once I will have pushed it.

> +// Tables used for numbering in persian words
> +static sal_Unicode *table_PersianWord_decade1[]={
> +    (sal_Unicode[]){0}, // 0
> +    (sal_Unicode[]){0x06cc, 0x06a9, 0}, // 1

I introduced a
typedef ArrUnicode sal_Unicode[];
and did
(ArrUnicode){...} for better legibility throught the entire patch. Also
nicely lined up the commented numbers in one column.

> +void lcl_formatPersianWord(unsigned int aNumber, OUString& rsResult)

Btw, I changed aNumber to nNumber to indicate the simple numeric type
instead of an object, that's one of our coding conventions.

> +{
> +    // Formats numbers in persian word up to 999999999999 which is high enough for showing int
> +    OUString asTemp;

Performance wise it pays to use an OUStringBuffer instead of an OUString
that is copied and recreated and reassigned all way long, initializing
the buffer with a sufficient capacity also gets rid of the memory
allocation bottlenecks. So, that now is

    OUStringBuffer aTemp(64);


> +    OUString asPersianWord_conjunction=OUString((sal_Unicode[]){0x20,0x0648,0x20,0});

<nitpick>
That can be constructed as

    OUString asPersianWord_conjunction( (ArrUnicode){0x20,0x0648,0x20,0});

without the assignment operator.
</nitpick>


> +    while(int anPart=aNumber%1000)

For example, that now is

    while (int nPart = nNumber % 1000)

looks much nicer, doesn't it? ;-)


To illustrate why to use an OUStringBuffer:

> +            
> asTemp=OUString(table_PersianWord_decadeX[anSection-1])+asPersianWord_conjunction+asTemp;

For that the compiler needs to

1. construct a temporary OUString instance of the expression
   OUString(table_PersianWord_decadeX[anSection-1])
2. construct and allocate a second temporary OUString for the
   concatenation of #1+asPersianWord_conjunction
3. construct and allocate a third temporary OUString for the
   concatenation of #2+asTemp
4. assign #3 to asTemp, hence deallocate the original asTemp and 
5. destruct all temporary instances

Instead,

            aTemp.insert( 0, asPersianWord_conjunction).insert( 0, 
table_PersianWord_decadeX[nSection-1]);

only needs to move some values for insert() in the preallocated buffer.
Note that insert() returns a OUStringBuffer& so while this looks
reversed at first hand the result is in the correct order. I changed all
those constructs to use insert() instead. Probably performance could be
enhanced if the algorithm would loop backwards and strings be appended
instead of inserted, but.. I don't think that's necessary here.


> +            if(!asTemp.isEmpty())

That then needs a change to

            if (aTemp.getLength())


> +                asTemp=OUString(0x0020)+asTemp;

Also this one can be written as

                aTemp.insert( 0, sal_Unicode(0x0020));

and so on..


> +    rsResult+=asTemp;

This one now is

    rsResult += aTemp.makeStringAndClear();


Thanks again, and keep on hacking ;)

  Eike

-- 
 PGP/OpenPGP/GnuPG encrypted mail preferred in all private communication.
 Key ID: 0x293C05FD - 997A 4C60 CE41 0149 0DB3  9E96 2F1A D073 293C 05FD

Attachment: signature.asc
Description: Digital signature