8300869: Make use of the Double.toString(double) algorithm in java.util.Formatter #12259

rgiulietti · 2023-01-27T16:02:38Z

Align double and float decimal conversions in java.util.Formatter with the algorithm used in Double.toString(double).

Progress

Change must be properly reviewed (1 review required, with at least 1 Reviewer)
Change must not contain extraneous whitespace
Commit message must refer to an issue
Change requires CSR request JDK-8301387 to be approved

Issues

JDK-8300869: Make use of the Double.toString(double) algorithm in java.util.Formatter
JDK-8301387: Align the specification of java.util.Formatter float conversions to the implementation (CSR)

Reviewers

Joe Darcy (@jddarcy - Reviewer) ⚠️ Review applies to 5e488a70
Naoto Sato (@naotoj - Reviewer)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk pull/12259/head:pull/12259
$ git checkout pull/12259

Update a local copy of the PR:
$ git checkout pull/12259
$ git pull https://git.openjdk.org/jdk pull/12259/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 12259

View PR using the GUI difftool:
$ git pr show -t 12259

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/12259.diff

…il.Formatter

bridgekeeper · 2023-01-27T16:04:22Z

👋 Welcome back rgiulietti! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

openjdk · 2023-01-27T16:06:53Z

@rgiulietti The following labels will be automatically applied to this pull request:

core-libs
i18n

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing lists. If you would like to change these labels, use the /label pull request command.

mlbridge · 2023-01-27T16:10:21Z

Webrevs

rgiulietti · 2023-01-27T16:15:18Z

The specification in Formatter explicitly refers to the outcome of Double.toString(double). However, it currently uses another implementation for the floating-point to decimal conversion, which might end up in rare, slightly different outcomes than the ones described in the specification.

This fixes the discrepancy between specification and implementation in Formatter.

Class jdk.internal.math.FormattedFPDecimal replaces jdk.internal.math.FormattedFloatingDecimal, which is no longer needed.

…il.Formatter Added tests for known differences between current and new behavior.

rgiulietti · 2023-01-27T18:22:11Z

Added tests for known differences.

Most of the modified files are generated automatically by a script that, somewhat questionably, renews the copyright year in some of the files for no apparent reason.

RogerRiggs · 2023-01-27T19:56:28Z

test/jdk/java/util/Formatter/BasicInt.java

@@ -1,5 +1,5 @@
 /*


It looks line the non-float/double test classes are unchanged, they could be dropped from the PR.

The only changes I made myself in the test files are in Basic-X.java.template (including the copyright year). The other files were generated by a script, which happens to also change the copyright year for otherwise unmodified files.

If you agree, I'll happily remove the trivial copyright year changes and only retain those files containing more substantial modifications.

Fine with me, I figured it was the script that changed the copyright and otherwise generated the same as before. tnx

Otherwise untouched files are now reverted back with previous copyright year

…il.Formatter Reverted back copyright year of otherwise untouched files.

…il.Formatter

jddarcy · 2023-01-30T18:39:31Z

I've spoken to @rgiulietti about this PR; since there behavioral changes of possible interest, I'm requesting a CSR to cover those changes.

jddarcy · 2023-01-30T18:39:38Z

/csr needed

jddarcy · 2023-01-30T18:47:31Z

test/jdk/java/util/Formatter/Basic-X.java.template

@@ -931,6 +931,16 @@ public class Basic$Type$ extends Basic {
        test("%3.0e", "1e+07",  10000000.00);
        test("%3.0e", "1e+08", 100000000.00);

+        //---------------------------------------------------------------------
+        // %e - adoption of Double.toString(double) algorithm (8300869)


Nit: we generally don't use the bug ids in comments like this, usually at most relying on a bug listed in the test "@bug" line.

The bug id was added there because I'm not sure there's a way to add the @bug line to Basic-X.java.template`.

I see; it might suffice in this case to rely on the bug information in the SCM history, but I don't have a strong opinion.

In this specific *.java.template case, I think that the bug id in the comment somehow replaces the lack of the @bug line.
But I have no problems in removing it if there are stronger opinions.

The @bug tag is in BasicTestLauncher which is where 8300869 should likely go.

Thanks @bplb for pointing to the right place.
Done

openjdk · 2023-01-30T18:58:18Z

@jddarcy this pull request will not be integrated until the CSR request JDK-8301387 for issue JDK-8300869 has been approved.

…il.Formatter

rgiulietti · 2023-01-30T19:45:00Z

The CSR has been added.

It covers both a behavioral change due to the implementation change in this PR, and a specification change to align it with the implementation (current and proposed), resolving a long standing mismatch in the e, f, and g conversions on float values.

…il.Formatter

jddarcy · 2023-01-31T22:19:25Z

src/java.base/share/classes/jdk/internal/math/FormattedFPDecimal.java

+
+package jdk.internal.math;
+
+public final class FormattedFPDecimal {


I suggest adding a short explanation of what this class is used for.

…il.Formatter

openjdk · 2023-02-01T20:48:06Z

@rgiulietti This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

8300869: Make use of the Double.toString(double) algorithm in java.util.Formatter

Reviewed-by: darcy, naoto

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 203 new commits pushed to the master branch:

c647ae6: 8301149: Parallel: Refactor MutableNUMASpace::update_layout
de57733: 8301644: com/sun/jdi/JdbStopThreadTest.java fails after JDK-8300811
930ec00: 8301636: Minor cleanup in CommentHelper and DocPretty
725d57b: 8301659: Resolve initialization reordering issues on Windows for libawt and libsaproc
2d50c7d: 8298979: Remove duplicated serviceability/jvmti/thread/GetAllThreads/allthr01/allthr01.java
59b7fb1: 8300273: [IR framework] Handle message instead of bailing out
5b1584b: 8298880: VectorLogicalOpIdentityTest.java IR test incorrectly use avx3 instead of avx512
21c1afb: 8301612: OopLoadProxy constructor should be explicit
03b23a1: 8301367: Add exception handler method to the BaseLdapServer
7b6ac41: 8286876: NMT.test_unaliged_block_address_vm_assert fails if using clang toolchain
... and 193 more: https://git.openjdk.org/jdk/compare/f307e8c667895c302e916124751456a5443353ce...master

As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

rgiulietti · 2023-02-02T09:44:13Z

Anyone from i18n wants to review before integration?

naotoj

I skimmed through the changes, and see no problem wrt the i18n area.

rgiulietti · 2023-02-02T19:08:47Z

/integrate

openjdk · 2023-02-02T19:10:58Z

Going to push as commit f696785.
Since your change was applied there have been 204 commits pushed to the master branch:

cf6b9eb: 8301637: ThreadLocalRandom.current().doubles().parallel() contention
c647ae6: 8301149: Parallel: Refactor MutableNUMASpace::update_layout
de57733: 8301644: com/sun/jdi/JdbStopThreadTest.java fails after JDK-8300811
930ec00: 8301636: Minor cleanup in CommentHelper and DocPretty
725d57b: 8301659: Resolve initialization reordering issues on Windows for libawt and libsaproc
2d50c7d: 8298979: Remove duplicated serviceability/jvmti/thread/GetAllThreads/allthr01/allthr01.java
59b7fb1: 8300273: [IR framework] Handle message instead of bailing out
5b1584b: 8298880: VectorLogicalOpIdentityTest.java IR test incorrectly use avx3 instead of avx512
21c1afb: 8301612: OopLoadProxy constructor should be explicit
03b23a1: 8301367: Add exception handler method to the BaseLdapServer
... and 194 more: https://git.openjdk.org/jdk/compare/f307e8c667895c302e916124751456a5443353ce...master

Your commit was automatically rebased without conflicts.

openjdk · 2023-02-02T19:11:25Z

@rgiulietti Pushed as commit f696785.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

8300869: Make use of the Double.toString(double) algorithm in java.ut…

bcbbe1e

…il.Formatter

openjdk bot added the rfr Pull request is ready for review label Jan 27, 2023

openjdk bot added core-libs core-libs-dev@openjdk.org i18n i18n-dev@openjdk.org labels Jan 27, 2023

8300869: Make use of the Double.toString(double) algorithm in java.ut…

d42fbd0

…il.Formatter Added tests for known differences between current and new behavior.

RogerRiggs reviewed Jan 27, 2023

View reviewed changes

rgiulietti added 2 commits January 27, 2023 23:18

8300869: Make use of the Double.toString(double) algorithm in java.ut…

6f026d4

…il.Formatter Reverted back copyright year of otherwise untouched files.

8300869: Make use of the Double.toString(double) algorithm in java.ut…

d559232

…il.Formatter

jddarcy reviewed Jan 30, 2023

View reviewed changes

openjdk bot added the csr Pull request needs approved CSR before integration label Jan 30, 2023

8300869: Make use of the Double.toString(double) algorithm in java.ut…

0c72b74

…il.Formatter

rgiulietti added 2 commits January 31, 2023 14:35

8300869: Make use of the Double.toString(double) algorithm in java.ut…

bfe3d15

…il.Formatter

8300869: Make use of the Double.toString(double) algorithm in java.ut…

5e488a7

…il.Formatter

jddarcy reviewed Jan 31, 2023

View reviewed changes

jddarcy approved these changes Jan 31, 2023

View reviewed changes

8300869: Make use of the Double.toString(double) algorithm in java.ut…

bf7d9f6

…il.Formatter

openjdk bot removed the csr Pull request needs approved CSR before integration label Feb 1, 2023

openjdk bot added the ready Pull request is ready to be integrated label Feb 1, 2023

naotoj approved these changes Feb 2, 2023

View reviewed changes

openjdk bot added the integrated Pull request has been integrated label Feb 2, 2023

openjdk bot closed this Feb 2, 2023

openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Feb 2, 2023

rgiulietti deleted the JDK-8300869 branch February 3, 2023 12:53

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

8300869: Make use of the Double.toString(double) algorithm in java.util.Formatter #12259

8300869: Make use of the Double.toString(double) algorithm in java.util.Formatter #12259

rgiulietti commented Jan 27, 2023 •

edited by openjdk bot

Loading

bridgekeeper bot commented Jan 27, 2023

openjdk bot commented Jan 27, 2023

mlbridge bot commented Jan 27, 2023 •

edited

Loading

rgiulietti commented Jan 27, 2023

rgiulietti commented Jan 27, 2023

RogerRiggs Jan 27, 2023

rgiulietti Jan 27, 2023

RogerRiggs Jan 27, 2023

rgiulietti Jan 27, 2023

jddarcy commented Jan 30, 2023

jddarcy commented Jan 30, 2023

jddarcy Jan 30, 2023

rgiulietti Jan 30, 2023

jddarcy Jan 30, 2023

rgiulietti Jan 30, 2023

bplb Jan 31, 2023

rgiulietti Jan 31, 2023

openjdk bot commented Jan 30, 2023

rgiulietti commented Jan 30, 2023

jddarcy Jan 31, 2023

rgiulietti Feb 1, 2023

openjdk bot commented Feb 1, 2023 •

edited

Loading

rgiulietti commented Feb 2, 2023

naotoj left a comment

rgiulietti commented Feb 2, 2023

openjdk bot commented Feb 2, 2023

openjdk bot commented Feb 2, 2023


		package jdk.internal.math;

		public final class FormattedFPDecimal {

8300869: Make use of the Double.toString(double) algorithm in java.util.Formatter #12259

8300869: Make use of the Double.toString(double) algorithm in java.util.Formatter #12259

Conversation

rgiulietti commented Jan 27, 2023 • edited by openjdk bot Loading

Progress

Issues

Reviewers

Reviewing

bridgekeeper bot commented Jan 27, 2023

openjdk bot commented Jan 27, 2023

mlbridge bot commented Jan 27, 2023 • edited Loading

Webrevs

rgiulietti commented Jan 27, 2023

rgiulietti commented Jan 27, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jddarcy commented Jan 30, 2023

jddarcy commented Jan 30, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

openjdk bot commented Jan 30, 2023

rgiulietti commented Jan 30, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

openjdk bot commented Feb 1, 2023 • edited Loading

rgiulietti commented Feb 2, 2023

naotoj left a comment

Choose a reason for hiding this comment

rgiulietti commented Feb 2, 2023

openjdk bot commented Feb 2, 2023

openjdk bot commented Feb 2, 2023

rgiulietti commented Jan 27, 2023 •

edited by openjdk bot

Loading

mlbridge bot commented Jan 27, 2023 •

edited

Loading

openjdk bot commented Feb 1, 2023 •

edited

Loading