On Tue, 23 Apr 2024 19:55:57 GMT, Anthony Scarpino <[email protected]>
wrote:
>> Volodymyr Paprotski has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> Comments from Jatin and Tony
>
> src/java.base/share/classes/sun/security/ec/ECOperations.java line 204:
>
>> 202: * @return the product
>> 203: */
>> 204: public MutablePoint multiply(AffinePoint affineP, byte[] s) {
>
> It seems like there could be some combining of both `multiply()`. If
> `multiply(AffinePoint, ...)` is called, it can call `DefaultMultiplier` with
> the `affineP`, but internally call the other `multiply(ECPoint, ...)` for the
> other situations. I'd rather not have two methods doing most of the same
> code, but different methods.
Thanks, they indeed look identical, didnt notice. Fixed. (repeated the same
hashmap refactoring and didnt notice I produced identical code twice)
> src/java.base/share/classes/sun/security/ec/ECOperations.java line 467:
>
>> 465: sealed static abstract class SmallWindowMultiplier implements
>> PointMultiplier
>> 466: permits DefaultMultiplier, DefaultMontgomeryMultiplier {
>> 467: private final AffinePoint affineP;
>
> I don't think `affineP` needs to be a class variable anymore. It's only used
> in the constructor
Didn't notice, thanks, fixed.
> src/java.base/share/classes/sun/security/ec/ECOperations.java line 592:
>
>> 590: }
>> 591:
>> 592: private final ProjectivePoint.Immutable[][] points;
>
> Can you define this at the top please.
Done
> src/java.base/share/classes/sun/security/ec/ECOperations.java line 668:
>
>> 666: }
>> 667:
>> 668: private final BigInteger[] base;
>
> Can you define this at the top. You use it in the constructor but it's
> defined later on.
Done
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/18583#discussion_r1578117929
PR Review Comment: https://git.openjdk.org/jdk/pull/18583#discussion_r1578147190
PR Review Comment: https://git.openjdk.org/jdk/pull/18583#discussion_r1578148562
PR Review Comment: https://git.openjdk.org/jdk/pull/18583#discussion_r1578150303
