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-boun...@openjdk.java.net> on behalf of Raffaello
Giulietti <raffaello.giulie...@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.giulie...@gmail.com
<mailto:raffaello.giulie...@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().
