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().
