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.
> + 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.
There are many more places in the code. What you've done is nice, but
it could be exemplary.
--
Andrew Haley
Java Platform Lead Engineer
Red Hat UK Ltd. <https://www.redhat.com>
EAC8 43EB D3EF DB98 CC77 2FAD A5CD 6035 332F A671
