173408 - Bug fix for Ethiopic Number Conversion Algorithm

Status: RESOLVED FIXED

(Core :: Internationalization, defect)

Description

[Daniel Yacob]

The existing algorithm in mozilla trips over certain cases where large numbers
have big sequences of zeros and doesn't always remove superfluous '1's when it
should.  A patch is provided to fix this along with a test page.

Comment 1

[Daniel Yacob]

Attachment: cvs diff -u against nsBulletFrame.cpp

Once applied this patch will need some tweaking.  I don't know how the ns
string class works and made some guesses.  The routine adjustments are modelled
after: http://www.ethiopic.org/Numerals/ArabicToEthiopic.c  which implements
the algorithm (recently updated) http://www.ethiopic.org/Numerals/

Comment 2

[Daniel Yacob]

Attachment: Tests for 90 numbers.

Some of these test case will probably fail just because the list item value
overflows the data type used for integers.

Comment 3

[Madhur Bhatia]

==> internationalization 

Comment 4

[Hixie (not reading bugmail)]

Given that I don't actually understand the algorithm yet (I am discussing this
with Daniel by e-mail) I don't think I should review it... at least not yet. :-)

Comment 5

[Daniel Yacob]

Attachment: Refreshed patch with 23/01/03 CVS snapshot

This patch replaces the previous.  It is identical except updated for todays
nsBulletFrame.cpp.

Comment 6

[Roy Yokoyama]

simon: would you look at the latest patch?

Comment 7

[Simon Montagu :smontagu]

Comment on attachment 112414 [details] [diff] [review]
Refreshed patch with 23/01/03 CVS snapshot

>+  if ( (n % 2) == 0 ) {  // number length was odd
>+    // precondition the string to always have the leading tens place
>+    // populated, this avoids a check within the loop:
>+
>+    //  I have no idea if this prefixing works, but you get the idea..
>+    asciiNumberString = (nsAutoString)"0" + asciiNumberString;
>+    n++;
>+  }

  asciiNumberString.Insert("0", 0) is the way to do this, but I think it would
be more efficient to initialize asciiNumberString to "0" before the call to
DecimalToText() and then use the substring from 1 to the end if the length of
the string is odd.

>+  for (PRInt32 place = n; place >= n; place--) {

This should be place >= 0, no?

>+    PRUnichar asciiTen = asciiNumberString.CharAt(n-place--);
>+    PRUnichar asciiOne = asciiNumberString.CharAt(n-place);

Now I'm confused. Are you iterating from highest digit to lowest digit or from
lowest to highest? The first version of the patch has a comment 
+  // Iterate from the highest digit to lowest digit
but this version keeps the original comment
   // Iterate from the lowest digit to higher digits

If you are going from high to low, please change the comment.

>+    // pos indicates if our grouping subscript is even or odd
>+    PRInt32 pos = (place % 4) / 2;
>+
>+    // find a separator, if any, to follow Ethiopic ten and one
>+    PRUnichar sep
>+    = ( place )
>+      ? ( pos )
>+        ? ( ethioOne || ethioTen )
>+          ? (PRUnichar) 0x137B
>+          : 0x0
>+        : (PRUnichar) 0x137C
>+      : 0x0
>+    ;

ethioOne and ethioTen are used here before they are defined. 
Triple nesting of [? :] ternaries is going too far, IMO.

>+      // map onto Ethiopic "tens": U+1372=Ethiopic number ten
>+      ethioTen = (PRUnichar) ((PRInt32) asciiTen +  0x1372 - 0x31); 
>+    }
>+    if (asciiOne > '0') 
>+    {
>+      //map onto Ethiopic "ones": 0x1369=Ethiopic digit one
>+      ethioOne = (PRUnichar) ((PRInt32) asciiOne + 0x1369 - 0x31); 

Use #defined constants instead of the hardcoded numbers (and maybe also instead
of the hardcoded literal '0' and '1').

Nit: "tens" and "units" is better English than "tens" and "ones".

>+    result.Insert(ethioNumber, 0);

If you are going from highest digit to lowest, then you want to use Append(),
not Insert().

Comment 8

[Daniel Yacob]

Attachment: Refereshed patch with 12/06/03 CVS Snapshot

This patch should contain all of the suggestions made by simon.  Please review
to see that I have understood the suggestions as intended.

Comment 9

[Simon Montagu :smontagu]

Attachment: Simplified version

I had to make a few changes to make the patch compile, and while I was at it I
made the names more descriptive, and made the logic a bit clearer, at least in
my eyes.

One of my earlier comments was wrong: it doesn't work to initialize
asciiNumberString to "0", because we then get the leading zero for values where
we bail out of the algorithm, e.g. "0-1" for -1.

The testcases all seem to work now, except when the values overflow.

Comment 10

[Simon Montagu :smontagu]

Attachment: Output of the testcase with the last patch

The testcase is too large for a screenshot. I hope a .pdf will work.

Comment 11

[Daniel Yacob]

Simon, it looks really super!  I'll print and examine the list carefully.  On 
the overflow issue, what is the highest number that mozilla can handle in 
lists?

Comment 12

[Simon Montagu :smontagu]

>what is the highest number that mozilla can handle in lists?

The list index is passed around in a PRInt32, so the maximum value is 2147483647
(0x7FFFFFFF)

Comment 13

[Simon Montagu :smontagu]

Comment on attachment 125863 [details] [diff] [review]
Simplified version

r=smontagu (I attached this, but it's in effect Daniel's patch incorporating my
review comments). jag, can you sr?

Comment 14

[jag (Peter Annema)]

Comment on attachment 125863 [details] [diff] [review]
Simplified version

sr=jag

Comment 15

[Simon Montagu :smontagu]

Fix checked in.

Comment 16

[Matthias Versen [:Matti]]

Comment on attachment 102295 [details] [diff] [review]
cvs diff -u against nsBulletFrame.cpp

clearing obsolete review request