On 25/04/14 13:12, Paul Sandoz wrote:
On Apr 23, 2014, at 11:06 AM, Peter Levart <[email protected]> wrote:
Hi,
I propose a patch for:
https://bugs.openjdk.java.net/browse/JDK-8040892
+1, nice use of putIfAbsent. Just double checking, did you run the stream unit
tests?
Agreed. This looks good to me.
Trivially, is this comment, in uniqKeysMapMerger, left over from a
previous cut'n'paste ?
144 * {@link Map#merge(Object, Object, BiFunction) Map.merge()}
-Chris.
I think key step was to separate map merge and accumulate functions for the
toMap* methods not accepting a value merge function. Now that is the case one
could still use merge, but that would be less efficient than putIfAbsent since
that does not require a lambda capturing the key.
That fix caused me to think that we should update the value merge function
accepting toMap* methods for the case when the merge function returns null, as
removing an entry would be most undesirable.
A simple fix would be to throw an NPE if the merge function returns null, if it
is acceptable to induce a side-effect of a removal, which seems reasonable
given a partial result would any be returned if a null check is performed on
the result of the merge function, otherwise that merge function requires
wrapping e.g. mergeFunction.andThen(Objects::requireNonNull).
Paul.
Here's the webrev:
http://cr.openjdk.java.net/~plevart/jdk9-dev/Collectors.duplicateKey/webrev.02/
There has already been a discussion on the lambda-dev about this bug:
http://mail.openjdk.java.net/pipermail/lambda-dev/2014-April/012005.html
Initially a fix has been proposed which used a private RuntimeException subclass thrown
by the merger function in order to transport the values that attempted to be merged out
of Map.merge() method to an outer context where they could be combined with the key to
produce a friendlier "duplicate key" message. It turns out that there is an
alternative approach that doesn't require exception acrobatics - using Map.putIfAbsent()
instead of Map.merge(). I checked both methods in Map defaults and in HashMap
implementations for possible semantic differences. Same behaviour can be achieved by
requiring that the value passed to Map.putIfAbsent(key, value) is non-null and throwing
NPE if it isn't.
Regards, Peter
