RFR: 8021591 : (s) Additional explicit null checks

[Paul Sandoz]

Hi Mike,

V. quick review 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?


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