<html>
<head>
<meta http-equiv="Content-Type" content="text/html; charset=UTF-8">
</head>
<body text="#000000" bgcolor="#FFFFFF">
Hi Adam, comments/questions below (mostly simple stuff, nothing
major):<br>
<ul>
<li>IntegerPolynomial.java</li>
<ul>
<li>The comment block for multByInt should reflect your changes
you made, namely the removal of "r" from the signature.</li>
<li>88: In the case of multiplying two very large long values
would we ever see those cause an overflow. If so, is it OK to
have them overflow before the reduce operation?<br>
</li>
<li>420-425: Looks like this was a copy of the comment block
from conditionalSwap(). Maybe needs to be tailored to what
the conditionalAssign method does?</li>
<li>IntegerPolynomialP256.java, IntegerPolynomialP384.java,
IntegerPolynomialP521.java</li>
<ul>
<li>In the carryReduce0 and carryReduce methods, you have many
uses of an integer literal (33554432 for P256, 134217728 for
P384/521). Should these be made as private static final
long values?</li>
</ul>
<li>P256OrderField.java, P384OrderField.java,
P521OrderField.java</li>
<ul>
<li>Similar question about using a static final long vs.
repeated int literal values in the carryReduce* methods.</li>
</ul>
<li>With respect to the last two main bullet items: Would
conversion to a static final value be difficult due to the
fact that they are generated from Fieldgen.jsh?<br>
</li>
</ul>
</ul>
<p>--Jamil<br>
</p>
<div class="moz-cite-prefix">On 11/30/2018 11:59 AM, Adam Petcher
wrote:<br>
</div>
<blockquote type="cite"
cite="mid:a714a487-879e-598d-dc46-18a75d18e60d@oracle.com">JBS:
<a class="moz-txt-link-freetext" href="https://bugs.openjdk.java.net/browse/JDK-8208648">https://bugs.openjdk.java.net/browse/JDK-8208648</a>
<br>
webrev: <a class="moz-txt-link-freetext" href="http://cr.openjdk.java.net/~apetcher/8208648/webrev.00/">http://cr.openjdk.java.net/~apetcher/8208648/webrev.00/</a>
<br>
<br>
This changeset includes the enhancements to the finite field
arithmetic library that are necessary for the new implementation
of ECDSA and ECDH[1]. The actual ECDH and ECDSA changes will be
reviewed separately. Please see the JBS tickets for more
information about the changes, and here are some additional notes:
<br>
<br>
1) This change includes a code generator that produces finite
field implementations. The generated code is included in the
review, and it will be checked in to the repository with a comment
indicating that it is generated and should not be modified
directly. Another option is to put the code generator into the
build process so the generated code does not need to be checked
in, but it's not clear whether this is a better option.
<br>
2) Reviewing every line of the generated code is probably not
necessary, and it is probably better to focus on the code
generator itself (Fieldgen.jsh). Of course, it is probably a good
idea to review the generated code for its structure and for
opportunities for improvement.
<br>
<br>
<br>
<br>
[1] <a class="moz-txt-link-freetext" href="https://bugs.openjdk.java.net/browse/JDK-8208698">https://bugs.openjdk.java.net/browse/JDK-8208698</a>
<br>
<br>
</blockquote>
<br>
</body>
</html>