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

[Jason Mehrens]

Maybe rename the catch variable from 'cause' or 'ignored' to 'unreachable' and then wrap java.io.IOException in java.io.IOError?

________________________________________
From: core-libs-dev <core-libs-dev-bounces at openjdk.java.net> on behalf of Raffaello Giulietti <raffaello.giulietti at gmail.com>
Sent: Thursday, April 18, 2019 3:37 PM
To: Brian Burkhalter; core-libs-dev
Subject: Re: [PATCH] 4511638: Double.toString(double) sometimes produces incorrect results

Hi,

On 18.04.19 21:29, Brian Burkhalter wrote:
>
>> On Apr 18, 2019, at 11:44 AM, Raffaello Giulietti
>> <raffaello.giulietti at gmail.com
>> <mailto:raffaello.giulietti at gmail.com>> wrote:
>>
>> here's another revision of the patch. Its purpose is to overcome the
>> test failure observed in [1]. To this end, the patch adds
>>
>> FloatToDecimal.appendTo(float, Appendable) and
>> DoubleToDecimal.appendTo(double, Appendable)
>>
>> static methods to help AbstractStringBuilder in using the corrected
>> algorithm implemented in
>>
>> FloatToDecimal.toString(float) and
>> DoubleToDecimal.toString(double), respectively.
>>
>> The implementation has been jmh tested to make sure there are no
>> performance regressions.
>
> Thanks, Raffaello.
>
>> As there are now only less than two months left before Rampdown 1 for
>> OpenJDK 13, I beg anybody interested in reviewing this patch to
>> contact me for any question or clarification. Also, you might want to
>> take a look at the CSR [2].
>
> +1 on both counts.
>
>> As usual, Brian will make the patch available as webrev in the coming
>> hours.
>
> Please see
>
> http://cr.openjdk.java.net/~bpb/4511638/webrev.03/
>
> I wonder whether in the new AbstractStringBuilder.append() changes the
> constructs:
> 880 try {
> 881 FloatToDecimal.appendTo(f, this);
> 882 } catch (IOException ignored) {
> 883 assert false;
> 884 }
> might be better as:
> 880 try {
> 881 FloatToDecimal.appendTo(f, this);
> 882 } catch (IOException cause) {
> 883 throw new RuntimeException(cause);
> 884 }
> Comments appreciated.
>
> Brian


The reason is that FloatToDecimal.appendTo() takes an Appendable as
parameter. Appendable's append() methods throw a checked IOException.
This cannot happen with AbstractStringBuilder (the this passed as
argument), so the code catches the IOException just to please the
compiler. The assert is there just in case, but control shall never pass
there. Whatever conventions are in place to emphasize that control flow
cannot reach some point, I'll adopt them. Let me know.

The same holds for DoubleToDecimal.appendTo().