On Mon, 30 Nov 2020 13:52:17 GMT, Pavel Rappo <pra...@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 > > @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)) I would even go as far as to rewrite that snippet like this: if (newValue == null) { remove(key); } else { put(key, newValue); } return newValue; This rewrite is possible thanks to the following properties of `Map.remove(Object key)`: 1. A call with an unmapped `key` has no effect. 2. A call with a mapped `key` has the same semantics regardless of the value that this key is mapped to. In particular, (2) covers `null` values. To me, this rewrite reads better; however, I understand that readability is subjective and that snippets used in `@implSpec` might be subject to additional requirements. ------------- PR: https://git.openjdk.java.net/jdk/pull/714