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? - * @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? +++ 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