On Sat, 7 Sep 2024 20:39:39 GMT, fabioromano1 <d...@openjdk.org> wrote:
>> This implementation of MutableBigInteger.leftShift(int) optimizes the >> current version, avoiding unnecessary copy of the MutableBigInteger's value >> content and performing the primitive shifting only in the original portion >> of the value array rather than in the value yet extended with trailing zeros. > > fabioromano1 has updated the pull request incrementally with two additional > commits since the last revision: > > - Merge branch 'patchLeftShift' of https://github.com/fabioromano1/jdk into > patchLeftShift > - Removed redundant code test/jdk/java/math/BigInteger/MutableBigIntegerShiftTests.java line 43: > 41: * @build java.base/java.math.MutableBigIntegerBox > 42: * @key randomness > 43: * @run junit/othervm -DmaxDurationMillis=3000 MutableBigIntegerShiftTests There's no need to have a max duration here, as there's no great risk of CPU consumption. Suggestion: * @run junit MutableBigIntegerShiftTests test/jdk/java/math/BigInteger/MutableBigIntegerShiftTests.java line 47: > 45: public class MutableBigIntegerShiftTests { > 46: > 47: private static final int DEFAULT_MAX_DURATION_MILLIS = 3_000; Remove test/jdk/java/math/BigInteger/MutableBigIntegerShiftTests.java line 52: > 50: static final int ORDER_MEDIUM = 100; > 51: > 52: private static int maxDurationMillis; Remove test/jdk/java/math/BigInteger/MutableBigIntegerShiftTests.java line 53: > 51: > 52: private static int maxDurationMillis; > 53: private static Random random = RandomFactory.getRandom(); Suggestion: private static final Random random = RandomFactory.getRandom(); test/jdk/java/math/BigInteger/MutableBigIntegerShiftTests.java line 55: > 53: private static Random random = RandomFactory.getRandom(); > 54: > 55: static boolean failure = false; Remove test/jdk/java/math/BigInteger/MutableBigIntegerShiftTests.java line 60: > 58: static void setMaxDurationMillis() { > 59: maxDurationMillis = Math.max(maxDurationMillis(), 0); > 60: } Remove test/jdk/java/math/BigInteger/MutableBigIntegerShiftTests.java line 65: > 63: for (int i = 0; i < 100; i++) { > 64: MutableBigIntegerBox x = fetchNumber(order); > 65: int n = Math.abs(random.nextInt() % 200); Suggestion: int n = random.nextInt(200); test/jdk/java/math/BigInteger/MutableBigIntegerShiftTests.java line 67: > 65: int n = Math.abs(random.nextInt() % 200); > 66: > 67: assertTrue(x.shiftLeft(n).compare Better to use `assertEquals()`: better messages in case of failure, with an "expected" and an "actual" value. test/jdk/java/math/BigInteger/MutableBigIntegerShiftTests.java line 71: > 69: "Inconsistent left shift: " + x + "<<" + n + " != " + > x + "*2^" + n); > 70: > 71: assertTrue(x.shiftLeft(n).shiftRight(n).compare(x) == 0, Same test/jdk/java/math/BigInteger/MutableBigIntegerShiftTests.java line 77: > 75: > 76: @Test > 77: public static void testShift() { A JUnit test should be non-static. test/jdk/java/math/BigInteger/MutableBigIntegerShiftTests.java line 79: > 77: public static void testShift() { > 78: shift(ORDER_SMALL); > 79: shift(ORDER_MEDIUM); JUnit has parameterized tests, which help to identify the parameter value in case of failure. test/jdk/java/math/BigInteger/MutableBigIntegerShiftTests.java line 107: > 105: int numInts = (order + 31) >> 5; > 106: int[] fullBits = new int[numInts]; > 107: for(int i = 0; i < numInts; i++) `Arrays.fill()`? test/jdk/java/math/BigInteger/MutableBigIntegerShiftTests.java line 153: > 151: > 152: return result; > 153: } I think this is getting too complex for a test, there's a risk that it is incorrect... Is there perhaps a way to have simpler code? test/jdk/java/math/BigInteger/MutableBigIntegerShiftTests.java line 163: > 161: return DEFAULT_MAX_DURATION_MILLIS; > 162: } > 163: } Remove ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/20008#discussion_r1754212513 PR Review Comment: https://git.openjdk.org/jdk/pull/20008#discussion_r1754214453 PR Review Comment: https://git.openjdk.org/jdk/pull/20008#discussion_r1754214570 PR Review Comment: https://git.openjdk.org/jdk/pull/20008#discussion_r1754222080 PR Review Comment: https://git.openjdk.org/jdk/pull/20008#discussion_r1754214679 PR Review Comment: https://git.openjdk.org/jdk/pull/20008#discussion_r1754214930 PR Review Comment: https://git.openjdk.org/jdk/pull/20008#discussion_r1754215136 PR Review Comment: https://git.openjdk.org/jdk/pull/20008#discussion_r1754215802 PR Review Comment: https://git.openjdk.org/jdk/pull/20008#discussion_r1754215995 PR Review Comment: https://git.openjdk.org/jdk/pull/20008#discussion_r1754216164 PR Review Comment: https://git.openjdk.org/jdk/pull/20008#discussion_r1754216332 PR Review Comment: https://git.openjdk.org/jdk/pull/20008#discussion_r1754216672 PR Review Comment: https://git.openjdk.org/jdk/pull/20008#discussion_r1754216920 PR Review Comment: https://git.openjdk.org/jdk/pull/20008#discussion_r1754217053
