On Mon, 30 Nov 2020 15:08:51 GMT, Pavel Rappo <pra...@openjdk.org> wrote:
>> @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. @pavelrappo The intended effect of the revised snippet is sensible, but again I note that it differs from the actual default implementation. Specifically: if the map does not contain the key and newValue is null, the default implementation currently does nothing, whereas the revised snippet calls `remove(key)`. This should have no effect _on the map_ but a subclass might override `remove` and this behavior difference is observable. (The typical example of this is maintaining a counter of the number of operations. I think _Effective Java_ uses that example in discussing subclassing.) I think the main priority here is fidelity to what the default implementation actually does -- at least, concerning operations on *this* -- and less on readability. ------------- PR: https://git.openjdk.java.net/jdk/pull/714