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. Although we should really have a conversation on code snippets in API specifications, this thread is not the place for that. However, I will minimally comment on some of what you've just said. 1. If a high-fidelity copy is not enough, an identical copy is required; that suggests a JavaDoc facility for embedding portions of code into doc comments. 2. I have to note that `Map.merge` (a method whose semantics is very close to that of `Map.compute`) is specified and implemented very similarly to what my [comment #1](https://github.com/openjdk/jdk/pull/714#issuecomment-735843488) proposed. > 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. Perhaps there's some confusion. If anything my [comment #2](https://github.com/openjdk/jdk/pull/714#issuecomment-735798573) was proposing to _remove_ an optimization carried over from the default implementation. ------------- PR: https://git.openjdk.java.net/jdk/pull/714