Re: Poor Javadoc of Map default methods [Re: RFR: 8029055: Map.merge must refuse null values]
See https://bugs.openjdk.java.net/browse/JDK-8029676
Stephen
On 26 November 2013 13:20, Stephen Colebourne scolebou...@joda.org wrote:
I took a quick look, but jumped back in horror at the start of the
Javadoc for the new methods in Map. A Javadoc description should start
with the positive, not the negative. I had to read the code to figure
out what they are supposed to do. Here are the four poor Javadocs and
some proposed enhacements:
merge()
If the specified key is not already associated with ...
should be
Updates the value for an existing key merging the existing value with
the specified value.
p
more detailed user-level explanation...
p
now the more spec/edge case part...
(if is a terrible way to start the docs. Say what it does!)
computeIfAbsent()
If the specified key is not already associated with a value (or is mapped...
should be
Associates the key with a value computed on demand if the key is not
currently present.
p
more detailed user-level explanation...
p
now the more spec/edge case part...
computeIfPresent()
If the value for the specified key is present and non-null,
should be
Updates the value for an existing key.
p
more detailed user-level explanation...
p
now the more spec/edge case part...
compute()
Attempts to compute a mapping for the specified key...
should be
Updates the value for a key, adding a new mapping if necessary.
p
more detailed user-level explanation...
p
now the more spec/edge case part...
(attempts is negative, not positive)
Similar problems seem to exist for putIfAbsent.
also...
I have to say that I don't overly like the method name.
map.merge(...) sounds like a bulk operation affecting the whole map
(ie. like putAll). Assuming that the method cannot be renamed, it
could be improved just by changing the method parameter names.:
default V merge(K key, V valueToMerge,
BiFunction? super V, ? super V, ? extends V mergeFunction) {
valueToMerge implies that action will occur to the parameter.
mergeFunction is much more meaningful that remapping Function.
Similar parameter name change, mappingFunction - valueCreationFunction
default V computeIfAbsent(
K key,
Function? super K, ? extends V valueCreationFunction) {
Similar parameter name change, remappingFunction - valueUpdateFunction
default V computeIfPresent(
K key,
BiFunction? super K, ? super V, ? extends V valueUpdateFunction) {
Similar parameter name change, remappingFunction - valueUpdateFunction
default V compute(
K key,
BiFunction? super K, ? super V, ? extends V valueUpdateFunction) {
also...
Can you use HashSet::new in the example code?
In general, all these new methods are written in a spec style, rather
than a user friendly style, and I'm afraid I don't think thats
sufficient.
Stephen
On 26 November 2013 04:32, Mike Duigou mike.dui...@oracle.com wrote:
On Nov 24 2013, at 16:31 , David Holmes david.hol...@oracle.com wrote:
Hi Mike,
There is still no clear spec for what should happen if the param value is
null.
I feel very uncomfortable the status quo of with null being ignored, used
for a sentinel and also as value. The relations between null and values in
this method are just too complicated.
Currently:
- The provided value may be either null or non-null. Is null a legal value?
It depends upon:
- Is there an existing value?
- Does the Map allow null values?
- Does the function allow null values?
- Existing null values are treated as absent.
- If a null value is passed should we remove this mapping or add it
to the map?
- null might not be accepted by the map
- The associated value would still be regarded as absent for
future operations.
- The function may return null to signal remove.
In particular I dislike adding a null entry to the map if there is no
current mapping (or mapped to null). It seems like it should be invariant
whether we end up with an associated value. If null is used to signify
remove then map.contains(key) will return variable results depending upon
the initial state. Having the behaviour vary based upon whether the map
allows null values would be even worse.
So I would like to suggest that we require value to be non-null. I have
provisionally updated the spec and impls accordingly.
The parenthesized part is wrong.
I think that's overzealous copying from compute(). I have removed it.
Also you have changed the method implementation not just the implDoc so the
bug synopsis is somewhat misleading.
I will correct this. More changes were made than I originally expected. New
synopsis will be Map.merge implementations should refuse null value param
I have updated the webrev.
http://cr.openjdk.java.net/~mduigou/JDK-8029055/1/webrev/
Thanks,
Mike
Thanks,
David
On 23/11/2013 10:25 AM, Mike Duigou wrote:
We'll be using
Re: Poor Javadoc of Map default methods [Re: RFR: 8029055: Map.merge must refuse null values]
On 26 November 2013 17:35, Martin Buchholz marti...@google.com wrote: I haven't looked in depth, but I agree with Stephen's analysis. This API and its javadoc needs work. E.g. It's not clear that the purpose of Map.compute is to *update* the mapping for key in the map. I actually felt that the names of all four methods felt wrong. compute and merge seem like unfortunate choices. Instead of The default implementation makes no guarantees about synchronization or atomicity properties of this method. we should boldly say that the default implementation is *not* atomic, even if the underlying map is. Saying that the default implementation is not atomic sounds uncontroversial. Stephen
Poor Javadoc of Map default methods [Re: RFR: 8029055: Map.merge must refuse null values]
I took a quick look, but jumped back in horror at the start of the
Javadoc for the new methods in Map. A Javadoc description should start
with the positive, not the negative. I had to read the code to figure
out what they are supposed to do. Here are the four poor Javadocs and
some proposed enhacements:
merge()
If the specified key is not already associated with ...
should be
Updates the value for an existing key merging the existing value with
the specified value.
p
more detailed user-level explanation...
p
now the more spec/edge case part...
(if is a terrible way to start the docs. Say what it does!)
computeIfAbsent()
If the specified key is not already associated with a value (or is mapped...
should be
Associates the key with a value computed on demand if the key is not
currently present.
p
more detailed user-level explanation...
p
now the more spec/edge case part...
computeIfPresent()
If the value for the specified key is present and non-null,
should be
Updates the value for an existing key.
p
more detailed user-level explanation...
p
now the more spec/edge case part...
compute()
Attempts to compute a mapping for the specified key...
should be
Updates the value for a key, adding a new mapping if necessary.
p
more detailed user-level explanation...
p
now the more spec/edge case part...
(attempts is negative, not positive)
Similar problems seem to exist for putIfAbsent.
also...
I have to say that I don't overly like the method name.
map.merge(...) sounds like a bulk operation affecting the whole map
(ie. like putAll). Assuming that the method cannot be renamed, it
could be improved just by changing the method parameter names.:
default V merge(K key, V valueToMerge,
BiFunction? super V, ? super V, ? extends V mergeFunction) {
valueToMerge implies that action will occur to the parameter.
mergeFunction is much more meaningful that remapping Function.
Similar parameter name change, mappingFunction - valueCreationFunction
default V computeIfAbsent(
K key,
Function? super K, ? extends V valueCreationFunction) {
Similar parameter name change, remappingFunction - valueUpdateFunction
default V computeIfPresent(
K key,
BiFunction? super K, ? super V, ? extends V valueUpdateFunction) {
Similar parameter name change, remappingFunction - valueUpdateFunction
default V compute(
K key,
BiFunction? super K, ? super V, ? extends V valueUpdateFunction) {
also...
Can you use HashSet::new in the example code?
In general, all these new methods are written in a spec style, rather
than a user friendly style, and I'm afraid I don't think thats
sufficient.
Stephen
On 26 November 2013 04:32, Mike Duigou mike.dui...@oracle.com wrote:
On Nov 24 2013, at 16:31 , David Holmes david.hol...@oracle.com wrote:
Hi Mike,
There is still no clear spec for what should happen if the param value is
null.
I feel very uncomfortable the status quo of with null being ignored, used for
a sentinel and also as value. The relations between null and values in this
method are just too complicated.
Currently:
- The provided value may be either null or non-null. Is null a legal value?
It depends upon:
- Is there an existing value?
- Does the Map allow null values?
- Does the function allow null values?
- Existing null values are treated as absent.
- If a null value is passed should we remove this mapping or add it
to the map?
- null might not be accepted by the map
- The associated value would still be regarded as absent for
future operations.
- The function may return null to signal remove.
In particular I dislike adding a null entry to the map if there is no current
mapping (or mapped to null). It seems like it should be invariant whether we
end up with an associated value. If null is used to signify remove then
map.contains(key) will return variable results depending upon the initial
state. Having the behaviour vary based upon whether the map allows null
values would be even worse.
So I would like to suggest that we require value to be non-null. I have
provisionally updated the spec and impls accordingly.
The parenthesized part is wrong.
I think that's overzealous copying from compute(). I have removed it.
Also you have changed the method implementation not just the implDoc so the
bug synopsis is somewhat misleading.
I will correct this. More changes were made than I originally expected. New
synopsis will be Map.merge implementations should refuse null value param
I have updated the webrev.
http://cr.openjdk.java.net/~mduigou/JDK-8029055/1/webrev/
Thanks,
Mike
Thanks,
David
On 23/11/2013 10:25 AM, Mike Duigou wrote:
We'll be using https://bugs.openjdk.java.net/browse/JDK-8029055 for this
issue.
I've posted a webrev here:
http://cr.openjdk.java.net/~mduigou/JDK-8029055/0/webrev/
There is an identical change in ConcurrentMap's merge().
Mike
