[PATCH] 4511638: Double.toString(double) sometimes produces incorrect results

[Raffaello Giulietti]

Hi Andrew,

In principle I agree with you.

However, in this case the maths underlying the algorithm to select the 
decimal are too involved to explain in comment form. I'm in the course 
of preparing a paper that explains the idea and the details. Then it 
should be easier to make sense out of the code.

Since to my knowledge the algorithm is novel, it will require some time 
for me to translate in paper form.

Other observations are interspersed with yours below.



On 2018-09-27 11:03, Andrew Haley wrote:
> On 09/26/2018 06:39 PM, raffaello.giulietti at gmail.com wrote:
> 
>> The submitted code contains both the changes to the current
>> implementation and extensive jtreg tests.
>>
>> While I've struggled to keep the code within the 80 chars/line limit,
>> mercurial still generates longer lines. Thus, to avoid possible problems
>> with the email systems, the code is submitted both inline and as an
>> attachment. Hope at least one copy makes its way without errors.
> 
> Overall, the commenting is much too light. There are many places
> where I think I know what you're doing but you don't explain it.
> 
> Here, for example:
> 
>> +
>> +        // pow5 = pow51*2^63 + pow50
>> +        long pow51 = ceilPow5dHigh(-k);
>> +        long pow50 = ceilPow5dLow(-k);
>> +
>> +        // p = p2*2^126 + p1*2^63 + p0 and p = pow5 * cb
>> +        long x0 = pow50 * cb;
>> +        long x1 = multiplyHigh(pow50, cb);
>> +        long y0 = pow51 * cb;
>> +        long y1 = multiplyHigh(pow51, cb);
>> +        long z = (x1 << 1 | x0 >>> 63) + (y0 & MASK_63);
>> +        long p0 = x0 & MASK_63;
>> +        long p1 = z & MASK_63;
>> +        long p2 = (y1 << 1 | y0 >>> 63) + (z >>> 63);
>> +        long vn = p2 << 1 + ord2alpha | p1 >>> 62 - ord2alpha;
>> +        if ((p1 & mask) != 0 || p0 >= threshold) {
>> +            vn |= 1;
>> +        }
> 
> ... etc. I think I can figure out what you're doing, but you could
> explain it.
> 
 > If you write the comments now while the code is still fresh in your
 > mind it'll be easy.
 >

There are some succinct comments here that explain the expected results. 
I'm not the kind of programmer that comments every line since here the 
mechanics is simple enough to follow in Java directly. A good 
explanation would either be mathematical, which requires better 
typography than US-ASCII, or some explanatory drawings.

What the semantics of vn, vnl and vnr are will be explained in the 
future paper mentioned above.



>> +    private static final long[] ceilPow5d = {
>> +        /* -292 */ 0x7FBB_D8FE_5F5E_6E27L, 0x497A_3A27_04EE_C3DFL,
>> +        /* -291 */ 0x4FD5_679E_FB9B_04D8L, 0x5DEC_6458_6315_3A6CL,
>> +        /* -290 */ 0x63CA_C186_BA81_C60EL, 0x7567_7D6E_7BDA_8906L,
>> +        /* -289 */ 0x7CBD_71E8_6922_3792L, 0x52C1_5CCA_1AD1_2B48L,
> 
> What exactly is this table anyway?  How was it generated?  Please say.
> 

The comments of the accessor methods that make use of this private table 
implicitly explain its semantics as well. I will add a comment to the 
field that refers to the comments in the methods.


How the table was generated and thoroughly verified is currently not 
part of my contribution to OpenJDK, not because it is something secret 
or complex but because I think it is irrelevant here.

Besides, where would the generator/verifier code be placed in the 
codebase? It would be completely detached from, and unrelated to, 
everything else. But maybe there is already some mechanism in place for 
similar "bootstrapping" code in the OpenJDK. Then I would like to know 
to consider adding the generator there.


> There are many more places in the code. What you've done is nice, but
> it could be exemplary.
> 

As said, this will be part of a separate paper. Hope this helps for the 
moment.


Thanks for your interest
Raffaello