RFR 8015008: Primitive iterator over empty sequence, null consumer: forEachRemaining methods do not throw NPE

[Chris Hegarty]

Looks fine to me Paul.

On 29/05/2013 12:35, Paul Sandoz wrote:
> Hi,
>
> Please review these changes to j.u.PrimitiveIterator to ensure the default forEachRemaining methods consistently throw an NPE when the consumer is null.
>
> I almost produced a webrev for this, but i thought it was just about acceptable size-wise and i hope easy to review in textual form. If this is considered impolite, awkward to review etc please say so and i will produce a webrev.

I found it a little difficult to review as it lacked some context around 
the changes. I found myself having to track down the source and compare 
line numbers. Not a problem, just that you happen to mention it ;-)

-Chris

>
> Paul.
>
> # HG changeset patch
> # User psandoz
> # Date 1369825083 -7200
> # Node ID 7ded996200218791c885c0aae4c474066101c7bd
> # Parent  bfdc1ed75460c9e6869827cf9acabd4b1a5e9d29
> 8015008: Primitive iterator over empty sequence, null consumer: forEachRemaining methods do not throw NPE
> Reviewed-by:
>
> diff -r bfdc1ed75460 -r 7ded99620021 src/share/classes/java/util/PrimitiveIterator.java
> --- a/src/share/classes/java/util/PrimitiveIterator.java	Wed May 29 12:58:02 2013 +0200
> +++ b/src/share/classes/java/util/PrimitiveIterator.java	Wed May 29 12:58:03 2013 +0200
> @@ -91,6 +91,7 @@
>            * @throws NullPointerException if the specified action is null
>            */
>           default void forEachRemaining(IntConsumer action) {
> +            Objects.requireNonNull(action);
>               while (hasNext())
>                   action.accept(nextInt());
>           }
> @@ -123,6 +124,8 @@
>                   forEachRemaining((IntConsumer) action);
>               }
>               else {
> +                // The method reference action::accept is never null
> +                Objects.requireNonNull(action);
>                   if (Tripwire.ENABLED)
>                       Tripwire.trip(getClass(), "{0} calling PrimitiveIterator.OfInt.forEachRemainingInt(action::accept)");
>                   forEachRemaining((IntConsumer) action::accept);
> @@ -162,6 +165,7 @@
>            * @throws NullPointerException if the specified action is null
>            */
>           default void forEachRemaining(LongConsumer action) {
> +            Objects.requireNonNull(action);
>               while (hasNext())
>                   action.accept(nextLong());
>           }
> @@ -194,6 +198,8 @@
>                   forEachRemaining((LongConsumer) action);
>               }
>               else {
> +                // The method reference action::accept is never null
> +                Objects.requireNonNull(action);
>                   if (Tripwire.ENABLED)
>                       Tripwire.trip(getClass(), "{0} calling PrimitiveIterator.OfLong.forEachRemainingLong(action::accept)");
>                   forEachRemaining((LongConsumer) action::accept);
> @@ -232,6 +238,7 @@
>            * @throws NullPointerException if the specified action is null
>            */
>           default void forEachRemaining(DoubleConsumer action) {
> +            Objects.requireNonNull(action);
>               while (hasNext())
>                   action.accept(nextDouble());
>           }
> @@ -265,6 +272,8 @@
>                   forEachRemaining((DoubleConsumer) action);
>               }
>               else {
> +                // The method reference action::accept is never null
> +                Objects.requireNonNull(action);
>                   if (Tripwire.ENABLED)
>                       Tripwire.trip(getClass(), "{0} calling PrimitiveIterator.OfDouble.forEachRemainingDouble(action::accept)");
>                   forEachRemaining((DoubleConsumer) action::accept);
> diff -r bfdc1ed75460 -r 7ded99620021 test/java/util/Iterator/PrimitiveIteratorDefaults.java
> --- /dev/null	Thu Jan 01 00:00:00 1970 +0000
> +++ b/test/java/util/Iterator/PrimitiveIteratorDefaults.java	Wed May 29 12:58:03 2013 +0200
> @@ -0,0 +1,115 @@
> +/*
> + * Copyright (c) 2013, Oracle and/or its affiliates. All rights reserved.
> + * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
> + *
> + * This code is free software; you can redistribute it and/or modify it
> + * under the terms of the GNU General Public License version 2 only, as
> + * published by the Free Software Foundation.
> + *
> + * This code is distributed in the hope that it will be useful, but WITHOUT
> + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
> + * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License
> + * version 2 for more details (a copy is included in the LICENSE file that
> + * accompanied this code).
> + *
> + * You should have received a copy of the GNU General Public License version
> + * 2 along with this work; if not, write to the Free Software Foundation,
> + * Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA.
> + *
> + * Please contact Oracle, 500 Oracle Parkway, Redwood Shores, CA 94065 USA
> + * or visit www.oracle.com if you need additional information or have any
> + * questions.
> + */
> +
> +import org.testng.annotations.Test;
> +
> +import java.util.PrimitiveIterator;
> +import java.util.function.Consumer;
> +import java.util.function.DoubleConsumer;
> +import java.util.function.IntConsumer;
> +import java.util.function.LongConsumer;
> +
> +import static org.testng.Assert.assertNotNull;
> +import static org.testng.Assert.assertTrue;
> +
> +/**
> + * @test
> + * @run testng PrimitiveIteratorDefaults
> + * @summary test default methods on PrimitiveIterator
> + */
> + at Test
> +public class PrimitiveIteratorDefaults {
> +
> +    public void testIntForEachRemainingWithNull() {
> +        PrimitiveIterator.OfInt i = new PrimitiveIterator.OfInt() {
> +            @Override
> +            public int nextInt() {
> +                return 0;
> +            }
> +
> +            @Override
> +            public boolean hasNext() {
> +                return false;
> +            }
> +        };
> +
> +        executeAndCatch(() ->  i.forEachRemaining((IntConsumer) null));
> +        executeAndCatch(() ->  i.forEachRemaining((Consumer<Integer>) null));
> +    }
> +
> +    public void testLongForEachRemainingWithNull() {
> +        PrimitiveIterator.OfLong i = new PrimitiveIterator.OfLong() {
> +            @Override
> +            public long nextLong() {
> +                return 0;
> +            }
> +
> +            @Override
> +            public boolean hasNext() {
> +                return false;
> +            }
> +        };
> +
> +        executeAndCatch(() ->  i.forEachRemaining((LongConsumer) null));
> +        executeAndCatch(() ->  i.forEachRemaining((Consumer<Long>) null));
> +    }
> +
> +    public void testDoubleForEachRemainingWithNull() {
> +        PrimitiveIterator.OfDouble i = new PrimitiveIterator.OfDouble() {
> +            @Override
> +            public double nextDouble() {
> +                return 0;
> +            }
> +
> +            @Override
> +            public boolean hasNext() {
> +                return false;
> +            }
> +        };
> +
> +        executeAndCatch(() ->  i.forEachRemaining((DoubleConsumer) null));
> +        executeAndCatch(() ->  i.forEachRemaining((Consumer<Double>) null));
> +    }
> +
> +    private void executeAndCatch(Runnable r) {
> +        executeAndCatch(NullPointerException.class, r);
> +    }
> +
> +    private void executeAndCatch(Class<? extends Exception>  expected, Runnable r) {
> +        Exception caught = null;
> +        try {
> +            r.run();
> +        }
> +        catch (Exception e) {
> +            caught = e;
> +        }
> +
> +        assertNotNull(caught,
> +                      String.format("No Exception was thrown, expected an Exception of %s to be thrown",
> +                                    expected.getName()));
> +        assertTrue(expected.isInstance(caught),
> +                   String.format("Exception thrown %s not an instance of %s",
> +                                 caught.getClass().getName(), expected.getName()));
> +    }
> +
> +}
>