On 04/25/2014 03:43 PM, Chris Hegarty wrote:
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.
Hi Chris,
Thanks for spotting the left over line. I'll remove it.
Regards, Peter
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
