Hi Andrew,
In principle I agree with you.
However, in this case the maths underlying the algorithm to select the
decimal are too involved to explain in comment form. I'm in the course
of preparing a paper that explains the idea and the details. Then it
should be easier to make sense out of the code.
Since to my knowledge the algorithm is novel, it will require some time
for me to translate in paper form.
Other observations are interspersed with yours below.
On 2018-09-27 11:03, Andrew Haley wrote:
On 09/26/2018 06:39 PM, raffaello.giulie...@gmail.com wrote:
The submitted code contains both the changes to the current
implementation and extensive jtreg tests.
While I've struggled to keep the code within the 80 chars/line limit,
mercurial still generates longer lines. Thus, to avoid possible problems
with the email systems, the code is submitted both inline and as an
attachment. Hope at least one copy makes its way without errors.
Overall, the commenting is much too light. There are many places
where I think I know what you're doing but you don't explain it.
Here, for example:
+
+ // pow5 = pow51*2^63 + pow50
+ long pow51 = ceilPow5dHigh(-k);
+ long pow50 = ceilPow5dLow(-k);
+
+ // p = p2*2^126 + p1*2^63 + p0 and p = pow5 * cb
+ long x0 = pow50 * cb;
+ long x1 = multiplyHigh(pow50, cb);
+ long y0 = pow51 * cb;
+ long y1 = multiplyHigh(pow51, cb);
+ long z = (x1 << 1 | x0 >>> 63) + (y0 & MASK_63);
+ long p0 = x0 & MASK_63;
+ long p1 = z & MASK_63;
+ long p2 = (y1 << 1 | y0 >>> 63) + (z >>> 63);
+ long vn = p2 << 1 + ord2alpha | p1 >>> 62 - ord2alpha;
+ if ((p1 & mask) != 0 || p0 >= threshold) {
+ vn |= 1;
+ }
... etc. I think I can figure out what you're doing, but you could
explain it.
> If you write the comments now while the code is still fresh in your
> mind it'll be easy.
>
There are some succinct comments here that explain the expected results.
I'm not the kind of programmer that comments every line since here the
mechanics is simple enough to follow in Java directly. A good
explanation would either be mathematical, which requires better
typography than US-ASCII, or some explanatory drawings.
What the semantics of vn, vnl and vnr are will be explained in the
future paper mentioned above.
+ private static final long[] ceilPow5d = {
+ /* -292 */ 0x7FBB_D8FE_5F5E_6E27L, 0x497A_3A27_04EE_C3DFL,
+ /* -291 */ 0x4FD5_679E_FB9B_04D8L, 0x5DEC_6458_6315_3A6CL,
+ /* -290 */ 0x63CA_C186_BA81_C60EL, 0x7567_7D6E_7BDA_8906L,
+ /* -289 */ 0x7CBD_71E8_6922_3792L, 0x52C1_5CCA_1AD1_2B48L,
What exactly is this table anyway? How was it generated? Please say.
The comments of the accessor methods that make use of this private table
implicitly explain its semantics as well. I will add a comment to the
field that refers to the comments in the methods.
How the table was generated and thoroughly verified is currently not
part of my contribution to OpenJDK, not because it is something secret
or complex but because I think it is irrelevant here.
Besides, where would the generator/verifier code be placed in the
codebase? It would be completely detached from, and unrelated to,
everything else. But maybe there is already some mechanism in place for
similar "bootstrapping" code in the OpenJDK. Then I would like to know
to consider adding the generator there.
There are many more places in the code. What you've done is nice, but
it could be exemplary.
As said, this will be part of a separate paper. Hope this helps for the
moment.
Thanks for your interest
Raffaello
