RFR: 8021591 : (s) Additional explicit null checks

[Chris Hegarty]

On 29/07/2013 12:20, Paul Sandoz wrote:
> Hi Mike,
>
> V. quick review below.

The changes look ok to me. Some small comments below.

> On Jul 27, 2013, at 12:31 AM, Mike Duigou<mike.duigou at oracle.com>  wrote:
>
>> Hello all;
>>
>> This patch adds some missing checks for null that, according to interface contract, should be throwing NPE. It also improves the existing tests to check for these cases.
>>
>> http://cr.openjdk.java.net/~mduigou/JDK-8021591/0/webrev/
>>
>> The changes to src/share/classes/java/util/concurrent/ConcurrentHashMap.java will be synchronized separately with the jsr166 workspace. They are part of this review to avoid test failures.
>>
>
> diff --git a/src/share/classes/java/util/Map.java b/src/share/classes/java/util/Map.java
> --- a/src/share/classes/java/util/Map.java
> +++ b/src/share/classes/java/util/Map.java
> @@ -804,6 +804,10 @@
>        *     return false;
>        * }</pre>
>        *
> +     * @implNote The default implementation does not throw NullPointerException
> +     * for maps that do not support null values if oldValue is null unless
> +     * newValue is also null.
>
>
> Is that really more a clarification of the default impl specification?

I thought implNote was to be used for default implementations, but it 
appears that it is implSpec?

>
>
> -     * @throws NullPointerException if a specified key or value is null,
> +     * @throws NullPointerException if a specified key or newValue is null,
>        *         and this map does not permit null keys or values
> +     * @throws NullPointerException if oldValue is null and this map does not
> +     *         permit null values
> +     *         (<a href="Collection.html#optional-restrictions">optional</a>)
>
>
> More curious than anything else, is it fine to have two declarations of NPE here?

Having two @throws looks a little strange to me. Putting it together???

  * @throws NullPointerException if a specified key or newValue is null,
  *         and this map does not permit null keys or values. If
  *         oldValue is null and this map does not permit null values
  *         (<a href="Collection.html#optional-restrictions">optional</a>).

Is this even right? Should the implNote not be part of the NPE throws? 
Do you want replace to be used to put an initial value ( where 
previously unset (null) )?

-Chris.

>
>
> +++ b/src/share/classes/java/util/TreeMap.java
> @@ -948,6 +948,27 @@
>       }
>
>       @Override
> +    public synchronized boolean replace(K key, V oldValue, V newValue) {
> +        Entry<K,V>  p = getEntry(key);
> +        if (p!=null&&  Objects.equals(oldValue, p.value)) {
> +            p.value = newValue;
> +            return true;
> +        }
> +        return false;
> +    }
> +
> +    @Override
> +    public synchronized V replace(K key, V value) {
>
> Remove the synchronized?
>
>
> I might be missing something but i cannot see where ExtendsAbstractCollection is used.
>
> Paul.
>
>> Mike
>