Re: RFR: JDK-8325255 jdk.internal.util.ReferencedKeySet::add using wrong test [v3]
On Sun, 11 Feb 2024 17:41:14 GMT, ExE Boss wrote: >> Jim Laskey has updated the pull request incrementally with one additional >> commit since the last revision: >> >> Update ReferencedKeyTest.java > > src/java.base/share/classes/jdk/internal/util/ReferencedKeySet.java line 151: > >> 149: @Override >> 150: public boolean add(T e) { >> 151: return intern(e) == e; > > https://github.com/openjdk/jdk/pull/14684#discussion_r1485634936 >> While `intern(e) == e` is (mostly) sufficient, it will return a false >> positive when `add(e)` is called and `e` is already present in the map, the >> correctest implementation would be some internal API in `ReferencedKeyMap` >> for implementing `ReferencedKeySet::add`. Updated - PR Review Comment: https://git.openjdk.org/jdk/pull/17732#discussion_r1513458157
Re: RFR: JDK-8325255 jdk.internal.util.ReferencedKeySet::add using wrong test [v3]
On Tue, 6 Feb 2024 18:43:09 GMT, Jim Laskey wrote: >> Currently, add is returning intern(e) == null which will always be false. >> The correct test is intern(e) == e , that is, true when element is newly >> added. > > Jim Laskey has updated the pull request incrementally with one additional > commit since the last revision: > > Update ReferencedKeyTest.java src/java.base/share/classes/jdk/internal/util/ReferencedKeySet.java line 151: > 149: @Override > 150: public boolean add(T e) { > 151: return intern(e) == e; https://github.com/openjdk/jdk/pull/14684#discussion_r1485634936 > While `intern(e) == e` is (mostly) sufficient, it will return a false > positive when `add(e)` is called and `e` is already present in the map, the > correctest implementation would be some internal API in `ReferencedKeyMap` > for implementing `ReferencedKeySet::add`. - PR Review Comment: https://git.openjdk.org/jdk/pull/17732#discussion_r1485635132
Re: RFR: JDK-8325255 jdk.internal.util.ReferencedKeySet::add using wrong test [v3]
> Currently, add is returning intern(e) == null which will always be false. The > correct test is intern(e) == e , that is, true when element is newly added. Jim Laskey has updated the pull request incrementally with one additional commit since the last revision: Update ReferencedKeyTest.java - Changes: - all: https://git.openjdk.org/jdk/pull/17732/files - new: https://git.openjdk.org/jdk/pull/17732/files/b8d2d97d..51cf3e4a Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=17732&range=02 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=17732&range=01-02 Stats: 1 line in 1 file changed: 1 ins; 0 del; 0 mod Patch: https://git.openjdk.org/jdk/pull/17732.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/17732/head:pull/17732 PR: https://git.openjdk.org/jdk/pull/17732