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

[Raffaello Giulietti]

Hi,

the semantics of java.io.IOError is:
"Thrown when a serious I/O error has occurred"
which I guess is not appropriate here.


I think the best compromise is

880 try {
881     FloatToDecimal.appendTo(f, this);
882 } catch (IOException cause) {
883     throw new AssertionError("Code shall be unreachable", cause);
884 }

which is more explicit and doesn't rely on assertions being enabled.


Greetings
Raffaello



On 2019-04-19 19:21, Jason Mehrens wrote:
> 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().
> 
>