Re: RFR: 4511638: Double.toString(double) sometimes produces incorrect results [v14]
> On Jun 1, 2022, at 3:32 AM, Raffaello Giulietti wrote: > > On Tue, 31 May 2022 22:11:54 GMT, Brian Burkhalter wrote: > >>> Raffaello Giulietti has updated the pull request incrementally with one >>> additional commit since the last revision: >>> >>> 4511638: Double.toString(double) sometimes produces incorrect results >> >> src/java.base/share/classes/jdk/internal/math/FloatToDecimal.java line 97: >> >>> 95: private static final int MASK_28 = (1 << 28) - 1; >>> 96: >>> 97: private static final int NON_SPECIAL= 0; >> >> As these are shared with `DoubleToDecimal` would these constants be better >> moved to a common location, e.g., `MathUtils` whether converted to an `enum` >> or not? > > As long as the values are constant `ints`, moving them to `MathUtils` is less > robust. Changing any value would require remembering to recompile the > "clients". > The move would make sense if these were an enum. > > - > > PR: https://git.openjdk.java.net/jdk/pull/3402 Understood. All of that can wait until later.
Re: RFR: 4511638: Double.toString(double) sometimes produces incorrect results [v14]
On Jun 1, 2022, at 2:25 AM, Raffaello Giulietti
mailto:d...@openjdk.java.net>> wrote:
On Tue, 31 May 2022 21:57:44 GMT, Brian Burkhalter
mailto:b...@openjdk.org>> wrote:
Raffaello Giulietti has updated the pull request incrementally with one
additional commit since the last revision:
4511638: Double.toString(double) sometimes produces incorrect results
src/java.base/share/classes/jdk/internal/math/DoubleToDecimal.java line 118:
116: private int index;
117:
118: private DoubleToDecimal() {
Maybe add a comment like
/**
* Prevent instantiation.
*/
or
// Prevent instantiation of this class.
The constructor _is_ invoked, so the comment would be inappropriate.
-
PR: https://git.openjdk.java.net/jdk/pull/3402
Mea culpa.
Re: RFR: 4511638: Double.toString(double) sometimes produces incorrect results [v14]
On Tue, 31 May 2022 22:11:54 GMT, Brian Burkhalter wrote: >> Raffaello Giulietti has updated the pull request incrementally with one >> additional commit since the last revision: >> >> 4511638: Double.toString(double) sometimes produces incorrect results > > src/java.base/share/classes/jdk/internal/math/FloatToDecimal.java line 97: > >> 95: private static final int MASK_28 = (1 << 28) - 1; >> 96: >> 97: private static final int NON_SPECIAL= 0; > > As these are shared with `DoubleToDecimal` would these constants be better > moved to a common location, e.g., `MathUtils` whether converted to an `enum` > or not? As long as the values are constant `ints`, moving them to `MathUtils` is less robust. Changing any value would require remembering to recompile the "clients". The move would make sense if these were an enum. - PR: https://git.openjdk.java.net/jdk/pull/3402
Re: RFR: 4511638: Double.toString(double) sometimes produces incorrect results [v14]
On Tue, 31 May 2022 21:55:16 GMT, Brian Burkhalter wrote: >> Raffaello Giulietti has updated the pull request incrementally with one >> additional commit since the last revision: >> >> 4511638: Double.toString(double) sometimes produces incorrect results > > src/java.base/share/classes/jdk/internal/math/DoubleToDecimal.java line 97: > >> 95: private static final int MASK_28 = (1 << 28) - 1; >> 96: >> 97: private static final int NON_SPECIAL= 0; > > Would these constants be better as an enum? An enum would make much sense if it were used by other parts of the codebase, and then it would be moved to `MathUtils`. This might well happen in the near future, when this code could be enhanced to be used in formatting conversions, like in "`printf()`" and friends. - PR: https://git.openjdk.java.net/jdk/pull/3402
Re: RFR: 4511638: Double.toString(double) sometimes produces incorrect results [v14]
On Wed, 1 Jun 2022 09:21:43 GMT, Raffaello Giulietti
wrote:
>> src/java.base/share/classes/jdk/internal/math/DoubleToDecimal.java line 118:
>>
>>> 116: private int index;
>>> 117:
>>> 118: private DoubleToDecimal() {
>>
>> Maybe add a comment like
>>
>> /**
>> * Prevent instantiation.
>> */
>>
>> or
>>
>> // Prevent instantiation of this class.
>
> The constructor _is_ invoked, so the comment would be inappropriate.
... but I added a `throw` in the constructor of `MathUtils`
-
PR: https://git.openjdk.java.net/jdk/pull/3402
Re: RFR: 4511638: Double.toString(double) sometimes produces incorrect results [v14]
On Tue, 31 May 2022 21:57:44 GMT, Brian Burkhalter wrote:
>> Raffaello Giulietti has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> 4511638: Double.toString(double) sometimes produces incorrect results
>
> src/java.base/share/classes/jdk/internal/math/DoubleToDecimal.java line 118:
>
>> 116: private int index;
>> 117:
>> 118: private DoubleToDecimal() {
>
> Maybe add a comment like
>
> /**
> * Prevent instantiation.
> */
>
> or
>
> // Prevent instantiation of this class.
The constructor _is_ invoked, so the comment would be inappropriate.
-
PR: https://git.openjdk.java.net/jdk/pull/3402
Re: RFR: 4511638: Double.toString(double) sometimes produces incorrect results [v14]
On Tue, 31 May 2022 21:35:08 GMT, Brian Burkhalter wrote: >> Raffaello Giulietti has updated the pull request incrementally with one >> additional commit since the last revision: >> >> 4511638: Double.toString(double) sometimes produces incorrect results > > src/java.base/share/classes/jdk/internal/math/MathUtils.java line 38: > >> 36: * >> 37: * Giulietti, "The Schubfach way to render doubles", >> 38: * >> https://drive.google.com/file/d/1gp5xv4CAa78SVgCeWfGqqI4FfYYYuNFb > > Even though not public, should the reference use the `` tag and > perhaps be in a `@see` annotation? > > @see href=“https://drive.google.com/file/d/1gp5xv4CAa78SVgCeWfGqqI4FfYYYuNFb”> > The Schubfach way to render doubles These references are inside a normal multi-line comment, so I'm not sure that Javadoc or html tags or have a well-defined semantics there. - PR: https://git.openjdk.java.net/jdk/pull/3402
Re: RFR: 4511638: Double.toString(double) sometimes produces incorrect results [v14]
On Wed, 1 Jun 2022 08:56:38 GMT, Raffaello Giulietti wrote: >> src/java.base/share/classes/jdk/internal/math/MathUtils.java line 38: >> >>> 36: * >>> 37: * Giulietti, "The Schubfach way to render doubles", >>> 38: * >>> https://drive.google.com/file/d/1gp5xv4CAa78SVgCeWfGqqI4FfYYYuNFb >> >> Even though not public, should the reference use the `` tag and >> perhaps be in a `@see` annotation? >> >> @see > href=“https://drive.google.com/file/d/1gp5xv4CAa78SVgCeWfGqqI4FfYYYuNFb”> >> The Schubfach way to render doubles > > These references are inside a normal multi-line comment, so I'm not sure that > Javadoc or html tags or have a well-defined semantics there. I'm not sure about linking to a document on Google Drive, even from JDK internal classes. Is the paper uploaded to anywhere else that could be linked to instead? - PR: https://git.openjdk.java.net/jdk/pull/3402
Re: RFR: 4511638: Double.toString(double) sometimes produces incorrect results [v14]
On Tue, 31 May 2022 17:07:06 GMT, Raffaello Giulietti wrote: >> Hello, >> >> here's a PR for a patch submitted on March 2020 >> [1](https://cr.openjdk.java.net/~bpb/4511638/webrev.04/) when Mercurial was >> a thing. >> >> The patch has been edited to adhere to OpenJDK code conventions about >> multi-line (block) comments. Nothing in the code proper has changed, except >> for the addition of redundant but clarifying parentheses in some expressions. >> >> >> Greetings >> Raffaello > > Raffaello Giulietti has updated the pull request incrementally with one > additional commit since the last revision: > > 4511638: Double.toString(double) sometimes produces incorrect results src/java.base/share/classes/jdk/internal/math/DoubleToDecimal.java line 72: > 70: static final int E_MAX = 309; > 71: > 72: /* Threshold to detect tiny values, as in section 8.2.1 of [1] */ Comments like this one pointing to specific places in the Schubfach paper are very helpful in understanding the constants and the various steps in the algorithm. - PR: https://git.openjdk.java.net/jdk/pull/3402
Re: RFR: 4511638: Double.toString(double) sometimes produces incorrect results [v14]
On Tue, 31 May 2022 17:07:06 GMT, Raffaello Giulietti
wrote:
>> Hello,
>>
>> here's a PR for a patch submitted on March 2020
>> [1](https://cr.openjdk.java.net/~bpb/4511638/webrev.04/) when Mercurial was
>> a thing.
>>
>> The patch has been edited to adhere to OpenJDK code conventions about
>> multi-line (block) comments. Nothing in the code proper has changed, except
>> for the addition of redundant but clarifying parentheses in some expressions.
>>
>>
>> Greetings
>> Raffaello
>
> Raffaello Giulietti has updated the pull request incrementally with one
> additional commit since the last revision:
>
> 4511638: Double.toString(double) sometimes produces incorrect results
src/java.base/share/classes/jdk/internal/math/FloatToDecimal.java line 40:
> 38: final public class FloatToDecimal {
> 39: /*
> 40: * For full details about this code see the following references:
Same comment about ``.
src/java.base/share/classes/jdk/internal/math/FloatToDecimal.java line 97:
> 95: private static final int MASK_28 = (1 << 28) - 1;
> 96:
> 97: private static final int NON_SPECIAL= 0;
As these are shared with `DoubleToDecimal` would these constants be better
moved to a common location, e.g., `MathUtils` whether converted to an `enum` or
not?
src/java.base/share/classes/jdk/internal/math/FloatToDecimal.java line 118:
> 116: private int index;
> 117:
> 118: private FloatToDecimal() {
Same comment about preventing instantiation.
-
PR: https://git.openjdk.java.net/jdk/pull/3402
Re: RFR: 4511638: Double.toString(double) sometimes produces incorrect results [v14]
On Tue, 31 May 2022 17:07:06 GMT, Raffaello Giulietti
wrote:
>> Hello,
>>
>> here's a PR for a patch submitted on March 2020
>> [1](https://cr.openjdk.java.net/~bpb/4511638/webrev.04/) when Mercurial was
>> a thing.
>>
>> The patch has been edited to adhere to OpenJDK code conventions about
>> multi-line (block) comments. Nothing in the code proper has changed, except
>> for the addition of redundant but clarifying parentheses in some expressions.
>>
>>
>> Greetings
>> Raffaello
>
> Raffaello Giulietti has updated the pull request incrementally with one
> additional commit since the last revision:
>
> 4511638: Double.toString(double) sometimes produces incorrect results
src/java.base/share/classes/jdk/internal/math/DoubleToDecimal.java line 97:
> 95: private static final int MASK_28 = (1 << 28) - 1;
> 96:
> 97: private static final int NON_SPECIAL= 0;
Would these constants be better as an enum?
src/java.base/share/classes/jdk/internal/math/DoubleToDecimal.java line 118:
> 116: private int index;
> 117:
> 118: private DoubleToDecimal() {
Maybe add a comment like
/**
* Prevent instantiation.
*/
or
// Prevent instantiation of this class.
-
PR: https://git.openjdk.java.net/jdk/pull/3402
Re: RFR: 4511638: Double.toString(double) sometimes produces incorrect results [v14]
On Tue, 31 May 2022 17:07:06 GMT, Raffaello Giulietti wrote: >> Hello, >> >> here's a PR for a patch submitted on March 2020 >> [1](https://cr.openjdk.java.net/~bpb/4511638/webrev.04/) when Mercurial was >> a thing. >> >> The patch has been edited to adhere to OpenJDK code conventions about >> multi-line (block) comments. Nothing in the code proper has changed, except >> for the addition of redundant but clarifying parentheses in some expressions. >> >> >> Greetings >> Raffaello > > Raffaello Giulietti has updated the pull request incrementally with one > additional commit since the last revision: > > 4511638: Double.toString(double) sometimes produces incorrect results src/java.base/share/classes/jdk/internal/math/DoubleToDecimal.java line 47: > 45: * [2] IEEE Computer Society, "IEEE Standard for Floating-Point > Arithmetic" > 46: * > 47: * [3] Bouvier & Zimmermann, "Division-Free Binary-to-Decimal > Conversion" Similar comment concerning `` tag as in `MathUtils`. - PR: https://git.openjdk.java.net/jdk/pull/3402
Re: RFR: 4511638: Double.toString(double) sometimes produces incorrect results [v14]
On Tue, 31 May 2022 17:07:06 GMT, Raffaello Giulietti
wrote:
>> Hello,
>>
>> here's a PR for a patch submitted on March 2020
>> [1](https://cr.openjdk.java.net/~bpb/4511638/webrev.04/) when Mercurial was
>> a thing.
>>
>> The patch has been edited to adhere to OpenJDK code conventions about
>> multi-line (block) comments. Nothing in the code proper has changed, except
>> for the addition of redundant but clarifying parentheses in some expressions.
>>
>>
>> Greetings
>> Raffaello
>
> Raffaello Giulietti has updated the pull request incrementally with one
> additional commit since the last revision:
>
> 4511638: Double.toString(double) sometimes produces incorrect results
src/java.base/share/classes/jdk/internal/math/MathUtils.java line 38:
> 36: *
> 37: * Giulietti, "The Schubfach way to render doubles",
> 38: *
> https://drive.google.com/file/d/1gp5xv4CAa78SVgCeWfGqqI4FfYYYuNFb
Even though not public, should the reference use the `` tag and perhaps
be in a `@see` annotation?
@see https://drive.google.com/file/d/1gp5xv4CAa78SVgCeWfGqqI4FfYYYuNFb”>
The Schubfach way to render doubles
src/java.base/share/classes/jdk/internal/math/MathUtils.java line 193:
> 191: */
> 192: private static final long[] g = {
> 193: /* -324 */ 0x4F0C_EDC9_5A71_8DD4L, 0x5B01_E8B0_9AA0_D1B5L,
Not significant, but might this be clearer instead to comment the array
elements on the right?
0x4F0C_EDC9_5A71_8DD4L, 0x5B01_E8B0_9AA0_D1B5L, // -324
-
PR: https://git.openjdk.java.net/jdk/pull/3402
Re: RFR: 4511638: Double.toString(double) sometimes produces incorrect results [v14]
> Hello, > > here's a PR for a patch submitted on March 2020 > [1](https://cr.openjdk.java.net/~bpb/4511638/webrev.04/) when Mercurial was a > thing. > > The patch has been edited to adhere to OpenJDK code conventions about > multi-line (block) comments. Nothing in the code proper has changed, except > for the addition of redundant but clarifying parentheses in some expressions. > > > Greetings > Raffaello Raffaello Giulietti has updated the pull request incrementally with one additional commit since the last revision: 4511638: Double.toString(double) sometimes produces incorrect results - Changes: - all: https://git.openjdk.java.net/jdk/pull/3402/files - new: https://git.openjdk.java.net/jdk/pull/3402/files/31ca4e10..ad146ec4 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=3402=13 - incr: https://webrevs.openjdk.java.net/?repo=jdk=3402=12-13 Stats: 19594 lines in 1 file changed: 0 ins; 19594 del; 0 mod Patch: https://git.openjdk.java.net/jdk/pull/3402.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/3402/head:pull/3402 PR: https://git.openjdk.java.net/jdk/pull/3402
