RFR: 8247402: Documentation for Map::compute contains confusing implementation requirements

[John Lin]

On Wed, 25 Nov 2020 16:39:20 GMT, Pavel Rappo <prappo at openjdk.org> wrote:

>> @pavelrappo Please see my proposed CSR below. Thank you.
>> 
>> # Map::compute should have a clearer implementation requirement.
>> 
>> ## Summary
>> 
>> java.util.Map::compute should have a clearer implementation requirement in its documentation.
>> 
>> ## Problem
>> 
>> The documentation of the implementation requirements for Map::compute has the following problems:
>> 1. It lacks of 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.
>> 
>> ## Solution
>> 
>> Rewrite the documentation of Map::compute to match its default implementation.
>> 
>> ## 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..c3118a90581 100644
>> --- a/src/java.base/share/classes/java/util/Map.java
>> +++ b/src/java.base/share/classes/java/util/Map.java
>> @@ -1113,17 +1113,12 @@ public interface Map<K, V> {
>>       * <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.remove(key);
>>       * }
>> +     * return newValue;
>>       * }</pre>
>>       *
>>       * <p>The default implementation makes no guarantees about detecting if the
>
> @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

-------------

PR: https://git.openjdk.java.net/jdk/pull/714