On Mon, 22 Aug 2022 07:35:28 GMT, Aleksey Shipilev <sh...@openjdk.org> wrote:
>> Daniel D. Daugherty has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> Refactor SpinedBufferTest.java by type.
>
> test/jdk/java/util/stream/boottest/java.base/java/util/stream/SpinedBufferTestCommon.java
> line 27:
>
>> 25: import java.util.*;
>> 26:
>> 27: public class SpinedBufferTestCommon {
>
> Usually done like this:
>
> Suggestion:
>
> public abstract class AbstractSpinedBufferTest {
fixed
> test/jdk/java/util/stream/boottest/java.base/java/util/stream/SpinedBufferTestCommon.java
> line 30:
>
>> 28:
>> 29: // Create sizes around the boundary of spines
>> 30: static List<Integer> sizes;
>
> Pre-existing, but code style:
>
> Suggestion:
>
> static final List<Integer> SIZES;
fixed even though it is pre-existing.
> test/jdk/java/util/stream/boottest/java.base/java/util/stream/SpinedBufferTestCommon.java
> line 43:
>
>> 41: catch (Exception e) {
>> 42: e.printStackTrace();
>> 43: }
>
> The usual way to terminate the clinit "cleanly" is:
>
> Suggestion:
>
> } catch (Exception e) {
> throw new IllegalStateException(e);
> }
>
>
> This allows you to `final` out the `SIZES` field, as dataflow would
> definitely show the class initialization fails if field init did not happen.
fixed
> test/jdk/java/util/stream/boottest/java.base/java/util/stream/SpinedBufferTestDouble.java
> line 35:
>
>> 33:
>> 34: @Test
>> 35: public class SpinedBufferTestDouble extends SpinedBufferTestCommon {
>
> Here and later, I think the common naming convention is having the "Test" at
> the end, i.e.:
>
> Suggestion:
>
> public class SpinedBufferDoubleTest extends SpinedBufferTestCommon {
fixed here and the other tests also.
> test/jdk/java/util/stream/boottest/java.base/java/util/stream/SpinedBufferTestDouble.java
> line 36:
>
>> 34: @Test
>> 35: public class SpinedBufferTestDouble extends SpinedBufferTestCommon {
>> 36: // DoubleSpinedBuffer
>
> Unnecessary comment, I think?
fixed here and the other tests also.
> test/jdk/java/util/stream/boottest/java.base/java/util/stream/SpinedBufferTestInt.java
> line 94:
>
>> 92: PrimitiveIterator.OfInt it = sb.iterator();
>> 93: for (int i = 0; i < TEST_SIZE; i++)
>> 94: list2.add(it.nextInt());
>
> Pre-existing? Here and later, let's keep the braces on:
>
> Suggestion:
>
> for (int i = 0; i < TEST_SIZE; i++) {
> list2.add(it.nextInt());
> }
fixed here and in other tests even though it is pre-existing.
-------------
PR: https://git.openjdk.org/jdk/pull/9845
