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

[Pavel Rappo]

On Sat, 21 Nov 2020 09:21:06 GMT, John Lin <github.com+1290376+johnlinp at openjdk.org> wrote:

>> @johnlinp, you cannot create a CSR by yourself at the moment. Someone else will have to do that for you. Might as well be me. So here's my proposal: come up with the meat, then I'll help you with the paperwork. 
>> 
>> For starters, have a look at existing CSRs (you don't need a JBS account for that). For example, https://bugs.openjdk.java.net/issues/?jql=issuetype%3DCSR%20and%20Subcomponent%3Djava.util%3Acollections%20
>> 
>> Fill in an informal CSR template inline in this thread, and we'll proceed from that point. Is that okay?
>
> @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

The proposed CSR has a few problems that we need to resolve.
1. The **Specification** pseudo-code behaves differently from both the old pseudo-code and the actual implementation when `newValue == null && oldValue == null` and `map.containsKey(key) == true`.
2. The content of the **Solution** section seems irrelevant: aside from a couple of missing `return` statements the current pseudo-code is fine. We are after something else, aren't we? The bottom line is we should state the solution more clearly.
3. The **Summary** section differs from that of the JDK-8247402.

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

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