Hi Anthony,
On 16/06/2019 19.17, Anthony Vanelverdinghe wrote:
Hi Raffaello
While I don't have feedback on the actual math, here's a few suggestions:
- there's some use of non-ASCII characters in the patch. I don't think
this is common in the JDK's Java sources, so you might want to replace
them with their Unicode escapes. The characters are: ≤ (\u2264), ∞
(\u221e), × (\u00d7), ≥ (\u2265), … (\u2026), ≠ (\u2260), ⌊(\u230a),
⌋(\u230b), · (\u00b7), β (\u03b2)
I'm not sure what you mean here: there are indeed usages of HTML
entities like ∞ that appear as math symbols in the rendered
Javadoc but the sources are 100% US-ASCII in my IDE and should be so
even in the patch.
If not, I'd be interested in the "coordinates" of the characters outside
of the US-ASCII set.
- there are 2 invocations of a deprecated String constructor for
performance reasons. I don't know how big the performance difference
is, but I would suggest replacing these invocations with `new
String(bytes, StandardCharsets.US_ASCII)` instead, and filing a bug
for the performance difference with the deprecated constructor
The perf diff is measurable. The chosen variant is the fastest that I
could come up with. As long as the deprecated constructor does not
become "forRemoval = true", if at all, I wouldn't worry. Even then,
there's plenty of time to switch.
Until then I can try to understand why the used constructor is faster
and perhaps file a bug as you suggest.
- there are 2 occurrences of a typo "left-to-tight"
Yep, thanks for your eagle's eyes!
Other than that, I can only say this is an impressive piece of work,
so I hope some official Reviewers will help you add your contribution
to the JDK.
The review process has indeed begun some days ago. However, since
there's is quite some maths to digest to understand the code, a full
review could take 1 to 2 days of concentrated reading: not the average
review, I guess.
Plus, there's a change in the specification, which must undergo a
further review.
Thanks for your time!
Greetings
Raffaello
Kind regards
Anthony
------------------------------------------------------------------------
*From:* core-libs-dev <core-libs-dev-boun...@openjdk.java.net> on
behalf of Raffaello Giulietti <raffaello.giulie...@gmail.com>
*Sent:* Monday, May 6, 2019 2:08:48 PM
*To:* core-libs-dev
*Subject:* [PATCH] 4511638: Double.toString(double) sometimes produces
incorrect results
Hi,
no new code this time, only a warm invitation to review the rather
substantial patch submitted on 2019-04-18 [1] and the CSR [2].
I spent an insane amount of (unpaid) free time offering my contribution
to solve a bug known since about 15 years. Thus, I think it is
understandable that I'd like to see my efforts come to a successful
conclusion, ideally for OpenJDK 13.
Greetings
Raffaello
P.S.
Some enjoyable properties of the novel algorithm:
* No objects are instantiated, except, of course, the resulting String.
* Loop-free core algorithm.
* Only int and long arithmetic.
* No divisions at all.
* 16x speedup (jmh).
* Randomized, yet reproducible deep diving tests (jtreg).
* Clear, unambiguous spec.
* All floats have been tested to fully meet the spec.
* Fully documented in [3] or in comments.
----
[1]
https://mail.openjdk.java.net/pipermail/core-libs-dev/2019-April/059783.html
[2] https://bugs.openjdk.java.net/browse/JDK-8202555
[3] https://drive.google.com/open?id=1KLtG_LaIbK9ETXI290zqCxvBW94dj058
