On Sat, 28 Nov 2020 08:42:52 GMT, John Lin
<github.com+1290376+johnl...@openjdk.org> wrote:
>> @johnlinp In any case, you'd also need to update the surrounding prose; we
>> need to remove this:
>> returning the current value or {@code null} if absent:```
>
> @pavelrappo Please see my updated CSR below. Thanks.
>
> # Map::compute should have the implementation requirement match its default
> implementation
>
> ## Summary
>
> The implementation requirement of Map::compute does not match its default
> implementation. Besides, it has some other minor issues. We should fix it.
>
> ## Problem
>
> The documentation of the implementation requirements for Map::compute has the
> following problems:
> 1. It doesn't match its default implementation.
> 1. It lacks of the return statements for most of the if-else cases.
> 1. The indents are 3 spaces, while the convention is 4 spaces.
> 1. The if-else is overly complicated and can be simplified.
> 1. The surrounding prose contains incorrect statements.
>
> ## Solution
>
> Rewrite the documentation of Map::compute to match its default implementation
> and solve the above mentioned problems.
>
> ## Specification
>
> diff --git a/src/java.base/share/classes/java/util/Map.java
> b/src/java.base/share/classes/java/util/Map.java
> index b1de34b42a5..b30e3979259 100644
> --- a/src/java.base/share/classes/java/util/Map.java
> +++ b/src/java.base/share/classes/java/util/Map.java
> @@ -1107,23 +1107,17 @@ public interface Map<K, V> {
> *
> * @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:
> + * steps for this {@code map}:
> *
> * <pre> {@code
> * V oldValue = map.get(key);
> * V newValue = remappingFunction.apply(key, oldValue);
> - * if (oldValue != null) {
> - * if (newValue != null)
> - * map.put(key, newValue);
> - * else
> - * map.remove(key);
> - * } else {
> - * if (newValue != null)
> - * map.put(key, newValue);
> - * else
> - * return null;
> + * if (newValue != null) {
> + * map.put(key, newValue);
> + * } else if (oldValue != null || map.containsKey(key)) {
> + * map.remove(key);
> * }
> + * return newValue;
> * }</pre>
> *
> * <p>The default implementation makes no guarantees about detecting if
> the
@johnlinp, thanks for updating the CSR draft; it is much better now.
@stuart-marks, I think we could further improve this snippet. This `if`
statement seems to use an optimization:
if (oldValue != null || map.containsKey(key))
I don't think we should include an optimization into the specification unless
that optimization also improves readability. Is this the case here? Could this
be better?
if (map.containsKey(key))
-------------
PR: https://git.openjdk.java.net/jdk/pull/714
