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

[Stuart Marks]

On Sat, 28 Nov 2020 08:42:52 GMT, John Lin <github.com+1290376+johnlinp at 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

The current snippet proposed by @johnlinp does seem to have the same behavior as the default implementation; I would avoid trying to "optimize" this. However, it does express the conditions and return value somewhat differently from the way the default implementation does. I think those differences are not significant to subclasses and are mostly stylistic. The original `@implSpec` snippet attempted to handle the cases separately, whereas the current proposed snippet minimizes them (while still agreeing with the implementation's behavior). I'm not too concerned about this. I think the current snippet is acceptable. Again, the main priority is agreement with the implementation.

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

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