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.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?
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
