Integrated: 8247402: Documentation for Map::compute contains confusing implementation requirements
On Sat, 17 Oct 2020 02:50:28 GMT, John Lin wrote: > This is from the mailing list: > http://mail.openjdk.java.net/pipermail/core-libs-dev/2020-June/067190.html > > - > ### Progress > - [x] Change must not contain extraneous whitespace > - [x] Commit message must refer to an issue > - [ ] Change must be properly reviewed > > ### Testing > > | | Linux x64 | Windows x64 | macOS x64 | > | --- | - | - | - | > | Build | ✔️ (5/5 passed) | ✔️ (2/2 passed) | ✔️ (2/2 passed) | > | Test (tier1) | ✔️ (9/9 passed) | ✔️ (9/9 passed) | ✔️ (9/9 passed) | > > > > ### Download > `$ git fetch https://git.openjdk.java.net/jdk pull/714/head:pull/714` > `$ git checkout pull/714` This pull request has now been integrated. Changeset: 37dc675c Author:John Lin Committer: Pavel Rappo URL: https://git.openjdk.java.net/jdk/commit/37dc675c Stats: 13 lines in 1 file changed: 1 ins; 7 del; 5 mod 8247402: Documentation for Map::compute contains confusing implementation requirements Reviewed-by: prappo, martin - PR: https://git.openjdk.java.net/jdk/pull/714
Re: RFR: 8247402: Documentation for Map::compute contains confusing implementation requirements [v2]
On Thu, 10 Dec 2020 17:59:03 GMT, Martin Buchholz wrote: >> John Lin has updated the pull request with a new target base due to a merge >> or a rebase. The pull request now contains one commit: >> >> 8247402: Documentation for Map::compute contains confusing implementation >> requirements > > Marked as reviewed by martin (Reviewer). Thank you all for the review and discussion. This is my first time contributing code in jdk, and it's a really nice experience. - PR: https://git.openjdk.java.net/jdk/pull/714
Re: RFR: 8247402: Documentation for Map::compute contains confusing implementation requirements
On Wed, 25 Nov 2020 16:39:20 GMT, Pavel Rappo wrote:
>> @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 {
>> * {@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;
>> * }
>> *
>> * The default implementation makes no guarantees about detecting if
>> the
>
> @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 {
*
* @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}:
*
* {@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;
* }
*
* The default implementation makes no guarantees about detecting if the
-
PR: https://git.openjdk.java.net/jdk/pull/714
Re: RFR: 8247402: Documentation for Map::compute contains confusing implementation requirements [v2]
> This is from the mailing list: > http://mail.openjdk.java.net/pipermail/core-libs-dev/2020-June/067190.html > > - > ### Progress > - [x] Change must not contain extraneous whitespace > - [x] Commit message must refer to an issue > - [ ] Change must be properly reviewed > > ### Testing > > | | Linux x64 | Windows x64 | macOS x64 | > | --- | - | - | - | > | Build | ✔️ (5/5 passed) | ✔️ (2/2 passed) | ✔️ (2/2 passed) | > | Test (tier1) | ✔️ (9/9 passed) | ✔️ (9/9 passed) | ✔️ (9/9 passed) | > > > > ### Download > `$ git fetch https://git.openjdk.java.net/jdk pull/714/head:pull/714` > `$ git checkout pull/714` John Lin has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains one additional commit since the last revision: 8247402: Documentation for Map::compute contains confusing implementation requirements - Changes: - all: https://git.openjdk.java.net/jdk/pull/714/files - new: https://git.openjdk.java.net/jdk/pull/714/files/d058e798..d16a35b8 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=714=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk=714=00-01 Stats: 394209 lines in 3620 files changed: 249166 ins; 107447 del; 37596 mod Patch: https://git.openjdk.java.net/jdk/pull/714.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/714/head:pull/714 PR: https://git.openjdk.java.net/jdk/pull/714
Re: RFR: 8247402: Documentation for Map::compute contains confusing implementation requirements
On Wed, 25 Nov 2020 20:22:21 GMT, Daniel Fuchs wrote:
>> @pavelrappo
>>
>>> What is the required level of fidelity particular (pseudo-) code has to
>>> have?
>>
>> It's potentially a large discussion, one that could be had in the context of
>> my JEP draft http://openjdk.java.net/jeps/8068562 . However, speaking
>> practically, it's possible to focus the discussion fairly concisely: the
>> main responsibility of the `@implSpec` ("Implementation Requirements")
>> section is to give implementors of subclasses enough information to decide
>> whether to inherit the implementation or to override it, and if they
>> override it, what behavior they can expect if they were to call
>> `super.compute`.
>>
>> In this case, a null-value-tolerating Map implementation needs to know that
>> the default implementation calls `remove` in the particular case that you
>> mentioned. A concurrent Map implementation will also need to know that the
>> default implementation calls `get(key)` and `containsKey(key)` at different
>> times, potentially leading to a race condition. Both of these inform the
>> override vs. inherit decision.
>
> @stuart-marks
>
>> Both of these inform the override vs. inherit decision.
>
> So in this case - fixing the specification to match the default
> implementation seems to be the right call - as existing implementations that
> do not override are more probably depending on the current default behavior.
Thank you all for giving me great advice. Sounds like the conclusion is to
update the documentation to match the default implementation. I'll update this
PR and propose a new CSR accordingly.
-
PR: https://git.openjdk.java.net/jdk/pull/714
Re: RFR: 8247402: Documentation for Map::compute contains confusing implementation requirements
On Wed, 18 Nov 2020 14:16:03 GMT, Pavel Rappo wrote:
>> @dfuch May I ask how can I create a CSR? I checked
>> https://wiki.openjdk.java.net/display/csr/CSR+FAQs and it says:
>>
>>> Q: How do I create a CSR ?
>>> A: Do not directly create a CSR from the Create Menu. JIRA will let you do
>>> this right up until the moment you try to save it and find your typing was
>>> in vain.
>>> Instead you should go to the target bug, select "More", and from the drop
>>> down menu select "Create CSR". This is required to properly associate the
>>> CSR with the main bug, just as is done for backports.
>>
>> However, I don't have an account at https://bugs.openjdk.java.net/ yet.
>> Therefore, I don't see the "More" button on
>> https://bugs.openjdk.java.net/browse/JDK-8247402. Could you please tell me
>> how do I proceed? Thank you.
>
> @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 {
* {@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;
* }
*
* The default implementation makes no guarantees about detecting if the
-
PR: https://git.openjdk.java.net/jdk/pull/714
Re: RFR: 8247402: Documentation for Map::compute contains confusing implementation requirements
On Wed, 18 Nov 2020 14:16:03 GMT, Pavel Rappo wrote: >> @dfuch May I ask how can I create a CSR? I checked >> https://wiki.openjdk.java.net/display/csr/CSR+FAQs and it says: >> >>> Q: How do I create a CSR ? >>> A: Do not directly create a CSR from the Create Menu. JIRA will let you do >>> this right up until the moment you try to save it and find your typing was >>> in vain. >>> Instead you should go to the target bug, select "More", and from the drop >>> down menu select "Create CSR". This is required to properly associate the >>> CSR with the main bug, just as is done for backports. >> >> However, I don't have an account at https://bugs.openjdk.java.net/ yet. >> Therefore, I don't see the "More" button on >> https://bugs.openjdk.java.net/browse/JDK-8247402. Could you please tell me >> how do I proceed? Thank you. > > @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 Thank you for your kind help and great proposal. I'll write up a draft CSR in this thread. - PR: https://git.openjdk.java.net/jdk/pull/714
Re: RFR: 8247402: Documentation for Map::compute contains confusing implementation requirements
On Tue, 20 Oct 2020 23:54:08 GMT, John Lin wrote: >> Hi Jon, >> >> Can you explain what this change is about: e.g. something like: >> >>> Updates the documentation of `Map::compute` to match its default >>> implementation: >>> The documentation of the default implementation of `Map::compute` was both >>> wrong and confusing. >>> This change updates the documentation to match the behaviour of the >>> implementation. >> >> because now I am confused. I believe what you are trying to do is what I >> have written above. Can you confirm? >> This will need a CSR. >> >> And are you going to withdraw https://github.com/openjdk/jdk/pull/451 now? >> >> best regards, >> -- daniel > > Hi @dfuch, > > Thanks for reviewing my PR. You're absolutely correct about what this PR is > about. Besides, I already closed #451. @dfuch May I ask how can I create a CSR? I checked https://wiki.openjdk.java.net/display/csr/CSR+FAQs and it says: > Q: How do I create a CSR ? > A: Do not directly create a CSR from the Create Menu. JIRA will let you do > this right up until the moment you try to > save it and find your typing was in vain. Instead you should go to the target > bug, select "More", and from the drop > down menu select "Create CSR". This is required to properly associate the CSR > with the main bug, just as is done for > backports. However, I don't have an account at https://bugs.openjdk.java.net/ yet. Therefore, I don't see the "More" button on https://bugs.openjdk.java.net/browse/JDK-8247402. Could you please tell me how do I proceed? Thank you. - PR: https://git.openjdk.java.net/jdk/pull/714
Re: RFR: 8247402: Documentation for Map::compute contains confusing implementation requirements
On Mon, 19 Oct 2020 17:26:56 GMT, Daniel Fuchs wrote: >> This is from the mailing list: >> http://mail.openjdk.java.net/pipermail/core-libs-dev/2020-June/067190.html >> >> - >> ### Progress >> - [x] Change must not contain extraneous whitespace >> - [x] Commit message must refer to an issue >> - [ ] Change must be properly reviewed >> >> ### Testing >> >> | | Linux x64 | Windows x64 | macOS x64 | >> | --- | - | - | - | >> | Build | ✔️ (5/5 passed) | ✔️ (2/2 passed) | ✔️ (2/2 passed) | >> | Test (tier1) | ✔️ (9/9 passed) | ✔️ (9/9 passed) | ✔️ (9/9 passed) | >> >> >> >> ### Download >> `$ git fetch https://git.openjdk.java.net/jdk pull/714/head:pull/714` >> `$ git checkout pull/714` > > Hi Jon, > > Can you explain what this change is about: e.g. something like: > >> Updates the documentation of `Map::compute` to match its default >> implementation: >> The documentation of the default implementation of `Map::compute` was both >> wrong and confusing. >> This change updates the documentation to match the behaviour of the >> implementation. > > because now I am confused. I believe what you are trying to do is what I have > written above. Can you confirm? > This will need a CSR. > > And are you going to withdraw https://github.com/openjdk/jdk/pull/451 now? > > best regards, > -- daniel Hi @dfuch, Thanks for reviewing my PR. You're absolutely correct about what this PR is about. Besides, I already closed #451. - PR: https://git.openjdk.java.net/jdk/pull/714
Withdrawn: 8247402: rewrite the implementation requirements for Map::compute()
On Thu, 1 Oct 2020 07:07:24 GMT, John Lin wrote: > This is from the mailing list: > http://mail.openjdk.java.net/pipermail/core-libs-dev/2020-June/067213.html This pull request has been closed without being integrated. - PR: https://git.openjdk.java.net/jdk/pull/451
RFR: 8247402: Documentation for Map::compute contains confusing implementation requirements
This is from the mailing list: http://mail.openjdk.java.net/pipermail/core-libs-dev/2020-June/067190.html - ### Progress - [x] Change must not contain extraneous whitespace - [x] Commit message must refer to an issue - [ ] Change must be properly reviewed ### Testing | | Linux x64 | Windows x64 | macOS x64 | | --- | - | - | - | | Build | ⏳ (3/5 running) | ⏳ (2/2 running) | ⏳ (2/2 running) | ### Download `$ git fetch https://git.openjdk.java.net/jdk pull/714/head:pull/714` `$ git checkout pull/714` - Commit messages: - 8247402: Documentation for Map::compute contains confusing implementation requirements Changes: https://git.openjdk.java.net/jdk/pull/714/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=714=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8247402 Stats: 11 lines in 1 file changed: 1 ins; 6 del; 4 mod Patch: https://git.openjdk.java.net/jdk/pull/714.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/714/head:pull/714 PR: https://git.openjdk.java.net/jdk/pull/714
Re: RFR: 8247402: rewrite the implementation requirements for Map::compute()
On Sat, 17 Oct 2020 02:40:35 GMT, John Lin
wrote:
>> This change is to a normative portion of the specification, and as such will
>> require a CSR.
>
> Hi @pavelrappo & @stuart-marks,
>
> Thank you for the review. When I first proposed this patch in the mailing
> list, the patch looked like this:
>
> # HG changeset patch
> # User John Lin
> # Date 1591923561 -28800
> # Fri Jun 12 08:59:21 2020 +0800
> # Node ID 03c9b5c9e632a0d6e33a1f13c98bb3b31b1bf659
> # Parent 49a68abdb0ba68351db0f140ddac793b1c391bd5
> 8247402: Rewrite the implementation requirements for Map::compute()
>
> diff --git a/src/java.base/share/classes/java/util/Map.java
> b/src/java.base/share/classes/java/util/Map.java
> --- 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 {
> * {@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;
> * }
> *
> * The default implementation makes no guarantees about detecting if
> the
>
> which didn't change the pseudo-code implementation.
>
> However, Martin Buchholz reviewed my patch and suggested me to modify it to
> match the real implementation in HashMap:
> https://www.mail-archive.com/core-libs-dev@openjdk.java.net/msg66197.html
> https://www.mail-archive.com/core-libs-dev@openjdk.java.net/msg66199.html
> https://www.mail-archive.com/core-libs-dev@openjdk.java.net/msg66210.html
> Therefore, I modified my patch and submitted it in the original mailing list
> and in this PR.
>
> I don't think creating a CSR is in the scope of JDK-8247402. I'll create
> another PR with my original patch. Thanks.
I've created https://github.com/openjdk/jdk/pull/714.
-
PR: https://git.openjdk.java.net/jdk/pull/451
Re: RFR: 8247402: rewrite the implementation requirements for Map::compute()
On Tue, 13 Oct 2020 16:17:49 GMT, Stuart Marks wrote:
>> Perhaps I should clarify my previous comment. When I said "implementation"
>> what I meant was that pseudo-code inside the
>> `@implSpec` tag, not the actual body of `default V compute(K key,
>> BiFunction
>> remappingFunction)`.
>
> This change is to a normative portion of the specification, and as such will
> require a CSR.
Hi @pavelrappo & @stuart-marks,
Thank you for the review. When I first proposed this patch in the mailing list,
the patch looked like this:
# HG changeset patch
# User John Lin
# Date 1591923561 -28800
# Fri Jun 12 08:59:21 2020 +0800
# Node ID 03c9b5c9e632a0d6e33a1f13c98bb3b31b1bf659
# Parent 49a68abdb0ba68351db0f140ddac793b1c391bd5
8247402: Rewrite the implementation requirements for Map::compute()
diff --git a/src/java.base/share/classes/java/util/Map.java
b/src/java.base/share/classes/java/util/Map.java
--- 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 {
* {@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;
* }
*
* The default implementation makes no guarantees about detecting if the
which didn't change the pseudo-code implementation.
However, Martin Buchholz reviewed my patch and suggested me to modify it to
match the real implementation in HashMap:
https://www.mail-archive.com/core-libs-dev@openjdk.java.net/msg66197.html
https://www.mail-archive.com/core-libs-dev@openjdk.java.net/msg66199.html
https://www.mail-archive.com/core-libs-dev@openjdk.java.net/msg66210.html
Therefore, I modified my patch and submitted it in the original mailing list
and in this PR.
I don't think creating a CSR is in the scope of JDK-8247402. I'll create
another PR with my original patch. Thanks.
-
PR: https://git.openjdk.java.net/jdk/pull/451
