On Jul 29 2013, at 04:20 , Paul Sandoz wrote:
> Hi Mike,
>
> V. quick review below.
>
>
> On Jul 27, 2013, at 12:31 AM, Mike Duigou <mike.dui...@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?
Yes. I will switch to @implSpec tag.
>
>
> - * @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?
Throwing NPE isn't optional if either key or newValue is null and the map does
not permit null values.
This restriction has to be softened for oldValue because the default
implementation doesn't test whether oldValue could be added to the map and
doesn't know if the map allows null values.
The multiple @throws approach is used as an alternative to a complicated
legalistic description in a single throws declaration. It's used uncommonly
elsewhere in the JDK.
> +++ 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?
Yes, thanks for catching that.
> I might be missing something but i cannot see where ExtendsAbstractCollection
> is used.
It may not be. I will check.
Mike
