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

[Martin Buchholz]

On Fri, 4 Dec 2020 16:54:26 GMT, Pavel Rappo <prappo at openjdk.org> wrote:

>> @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
>
> 1. @johnlinp I've created the CSR: https://bugs.openjdk.java.net/browse/JDK-8257768
> 2. @dfuch, @stuart-marks, and I-cannot-seem-to-find-Martin-Buchholz's-GitHub-username: care to review that CSR?

Hello github - this is my first ever comment.

The javadoc specs for the various compute methods in all the Map classes should be maintained consistently and holistically.  Sorry for not having done so years ago.

Pavel is thinking about code snippets for all of OpenJDK today, while I have been thinking about code snippets in jsr166.  jsr166 code snippets have a consistent style that should also be used for Map.java, but we've had a mild case of maintainer style divergence.

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

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