# HG changeset patch # User mduigou # Date 1386970495 28800 # Node ID d9836bf9992ae608265cf71b4f7aedbd920d3f4f # Parent 503bc3781dfe418b76e185e55342ebe0a9e20b30 8029055: Map.merge implementations should refuse null value param Reviewed-by: briangoetz, dl diff -r 503bc3781dfe -r d9836bf9992a jdk/src/share/classes/java/util/HashMap.java --- a/jdk/src/share/classes/java/util/HashMap.java Fri Dec 13 13:35:35 2013 -0800 +++ b/jdk/src/share/classes/java/util/HashMap.java Fri Dec 13 13:34:55 2013 -0800 @@ -1212,6 +1212,8 @@ @Override public V merge(K key, V value, BiFunction remappingFunction) { + if (value == null) + throw new NullPointerException(); if (remappingFunction == null) throw new NullPointerException(); int hash = hash(key); diff -r 503bc3781dfe -r d9836bf9992a jdk/src/share/classes/java/util/Map.java --- a/jdk/src/share/classes/java/util/Map.java Fri Dec 13 13:35:35 2013 -0800 +++ b/jdk/src/share/classes/java/util/Map.java Fri Dec 13 13:34:55 2013 -0800 @@ -600,7 +600,7 @@ * @implSpec * The default implementation is equivalent to, for this {@code map}: * 
 {@code
-     * for ((Map.Entry entry : map.entrySet())
+     * for (Map.Entry entry : map.entrySet())
      *     action.accept(entry.getKey(), entry.getValue());
      * }
* @@ -640,7 +640,7 @@ * @implSpec *

The default implementation is equivalent to, for this {@code map}: * 

 {@code
-     * for ((Map.Entry entry : map.entrySet())
+     * for (Map.Entry entry : map.entrySet())
      *     entry.setValue(function.apply(entry.getKey(), entry.getValue()));
      * }
* @@ -1110,8 +1110,8 @@ /** * If the specified key is not already associated with a value or is - * associated with null, associates it with the given value. - * Otherwise, replaces the value with the results of the given + * associated with null, associates it with the given non-null value. + * Otherwise, replaces the associated value with the results of the given * remapping function, or removes if the result is {@code null}. This * method may be of use when combining multiple mapped values for a key. * For example, to either create or append a {@code String msg} to a @@ -1121,15 +1121,14 @@ * map.merge(key, msg, String::concat) * } * - *

If the function returns {@code null}, the mapping is removed (or - * remains absent if initially absent). If the function itself throws an - * (unchecked) exception, the exception is rethrown, and the current mapping - * is left unchanged. + *

If the function returns {@code null} the mapping is removed. If the + * function itself throws an (unchecked) exception, the exception is + * rethrown, and the current mapping is left unchanged. * * @implSpec - * The default implementation is equivalent to performing the - * following steps for this {@code map}, then returning the - * current value or {@code null} if absent: + * The default implementation is equivalent to performing the following + * steps for this {@code map}, then returning the current value or + * {@code null} if absent: * * 

 {@code
      * V oldValue = map.get(key);
@@ -1137,8 +1136,6 @@
      *              remappingFunction.apply(oldValue, value);
      * if (newValue == null)
      *     map.remove(key);
-     * else if (oldValue == null)
-     *     map.remove(key);
      * else
      *     map.put(key, newValue);
      * }
@@ -1151,42 +1148,36 @@ * whether the function is applied once atomically only if the value is not * present. * - * @param key key with which the specified value is to be associated - * @param value the value to use if absent + * @param key key with which the resulting value is to be associated + * @param value the non-null value to be merged with the existing value + * associated with the key or, if no existing value or a null value + * is associated with the key, to be associated with the key * @param remappingFunction the function to recompute a value if present - * @return the new value associated with the specified key, or null if none + * @return the new value associated with the specified key, or null if no + * value is associated with the key * @throws UnsupportedOperationException if the {@code put} operation * is not supported by this map * (optional) * @throws ClassCastException if the class of the specified key or value * prevents it from being stored in this map * (optional) - * @throws NullPointerException if the specified key is null and - * this map does not support null keys, or the remappingFunction - * is null + * @throws NullPointerException if the specified key is null and this map + * does not support null keys or the value or remappingFunction is + * null * @since 1.8 */ default V merge(K key, V value, BiFunction remappingFunction) { Objects.requireNonNull(remappingFunction); + Objects.requireNonNull(value); V oldValue = get(key); - if (oldValue != null) { - V newValue = remappingFunction.apply(oldValue, value); - if (newValue != null) { - put(key, newValue); - return newValue; - } else { - remove(key); - return null; - } + V newValue = (oldValue == null) ? value : + remappingFunction.apply(oldValue, value); + if(newValue == null) { + remove(key); } else { - if (value == null) { - remove(key); - return null; - } else { - put(key, value); - return value; - } + put(key, newValue); } + return newValue; } } diff -r 503bc3781dfe -r d9836bf9992a jdk/src/share/classes/java/util/concurrent/ConcurrentMap.java --- a/jdk/src/share/classes/java/util/concurrent/ConcurrentMap.java Fri Dec 13 13:35:35 2013 -0800 +++ b/jdk/src/share/classes/java/util/concurrent/ConcurrentMap.java Fri Dec 13 13:34:55 2013 -0800 @@ -463,9 +463,9 @@ * {@inheritDoc} * * @implSpec - * The default implementation is equivalent to performing the - * following steps for this {@code map}, then returning the - * current value or {@code null} if absent: + * The default implementation is equivalent to performing the following + * steps for this {@code map}, then returning the current value or + * {@code null} if absent: * * 
 {@code
      * V oldValue = map.get(key);
@@ -473,8 +473,6 @@
      *              remappingFunction.apply(oldValue, value);
      * if (newValue == null)
      *     map.remove(key);
-     * else if (oldValue == null)
-     *     map.remove(key);
      * else
      *     map.put(key, newValue);
      * }
diff -r 503bc3781dfe -r d9836bf9992a jdk/test/java/util/Map/Defaults.java --- a/jdk/test/java/util/Map/Defaults.java Fri Dec 13 13:35:35 2013 -0800 +++ b/jdk/test/java/util/Map/Defaults.java Fri Dec 13 13:34:55 2013 -0800 @@ -767,7 +767,6 @@ Collection cases = new ArrayList<>(); cases.addAll(makeMergeTestCases()); - cases.addAll(makeMergeNullValueTestCases()); return cases.iterator(); } @@ -790,32 +789,6 @@ return cases; } - static Collection makeMergeNullValueTestCases() { - Collection cases = new ArrayList<>(); - - for( Object[] mapParams : makeAllRWMapsWithNulls() ) { - cases.add(new Object[] { mapParams[0], mapParams[1], Merging.Value.OLDVALUE, Merging.Value.NULL, Merging.Merger.NULL, Merging.Value.ABSENT, Merging.Value.NULL }); - } - - for( Object[] mapParams : makeAllRWMapsWithNulls() ) { - cases.add(new Object[] { mapParams[0], mapParams[1], Merging.Value.OLDVALUE, Merging.Value.NULL, Merging.Merger.RESULT, Merging.Value.RESULT, Merging.Value.RESULT }); - } - - for( Object[] mapParams : makeAllRWMapsWithNulls() ) { - cases.add(new Object[] { mapParams[0], mapParams[1], Merging.Value.ABSENT, Merging.Value.NULL, Merging.Merger.UNUSED, Merging.Value.ABSENT, Merging.Value.NULL }); - } - - for( Object[] mapParams : makeAllRWMapsWithNulls() ) { - cases.add(new Object[] { mapParams[0], mapParams[1], Merging.Value.NULL, Merging.Value.NULL, Merging.Merger.UNUSED, Merging.Value.ABSENT, Merging.Value.NULL }); - } - - for( Object[] mapParams : makeAllRWMapsWithNulls() ) { - cases.add(new Object[] { mapParams[0], mapParams[1], Merging.Value.NULL, Merging.Value.NEWVALUE, Merging.Merger.UNUSED, Merging.Value.NEWVALUE, Merging.Value.NEWVALUE }); - } - - return cases; - } - public interface Thrower { public void run() throws T;