Re: RFR: 8285295: Need better testing for IdentityHashMap [v2]
On Wed, 4 May 2022 15:02:43 GMT, liach wrote:
>> test/jdk/java/util/IdentityHashMap/Basic.java line 500:
>>
>>> 498: Box newKey = new Box(k1a);
>>> 499: Box newVal = new Box(v1a);
>>> 500: Box r = map.computeIfAbsent(newKey, k -> { called[0] = true;
>>> return newVal; });
>>
>> More of a curiosity than a review comment - I see that various places in
>> this PR use a boolean array with one element instead of just a boolean type.
>> Is that a personal coding preference or is there something more to it?
>
> This just serves as a modifiable boolean like an AtomicBoolean. Remember
> lambdas can only use final local var references (due to how they work), and
> it cannot access or modify the local variable in the caller method.
Thank you @liach and Stuart. I had overlooked the lambda aspect of this.
-
PR: https://git.openjdk.java.net/jdk/pull/8354
Re: RFR: 8285295: Need better testing for IdentityHashMap [v2]
On Wed, 4 May 2022 14:55:25 GMT, Jaikiran Pai wrote: >> Stuart Marks has updated the pull request incrementally with two additional >> commits since the last revision: >> >> - Assertions over return values. Some refinement of equals() testing. >> - Add comment about Map.Entry identity not guaranteed. > > test/jdk/java/util/IdentityHashMap/Basic.java line 50: > >> 48: // that a map's entrySet contains exactly the expected mappings. In most >> cases, keys and >> 49: // values should be compared by identity, not equality. However, the >> identities of Map.Entry >> 50: // instances from an IdentityHashSet are not guaranteed; the keys and >> values they contain > > I think I understand what you are saying here, but it took me a few reads to > understand it. More so because of `IdentityHashSet` here, which I think is a > typo. > So what's being stated here is that you cannot do something like: > > IdentityHashMap m = new IdentityHashMap(); > ... > var e1 = m.entrySet(); > var e2 = m.entrySet(); > > assertSame(e1, e2); // this isn't guaranteed to pass > > Did I understand this right? Yeah that comment was poorly worded, and indeed I meant the IdentityHashMap's entrySet and not IdentityHashSet. I've reworded it. I was trying to make a statement about exactly what needs to be compared if you want to make assertions about two maps being "equal" in the sense of IdentityHashMap behaving correctly. Specifically, given two maps `m1` and `m2`, clearly we don't want either of assertSame(m1, m2); assertSame(m1.entrySet(), m2.entrySet()); Instead, we want the `Map.Entry` instances to "match". However, given two entries `entry1` and `entry2` that are supposed to match, we also do not want assertSame(entry1, entry2); Instead, we want assertSame(entry1.getKey(), entry2.getKey()); assertSame(entry1.getValue(), entry2,getValue()); The `checkEntries` method goes to some length to get this right. - PR: https://git.openjdk.java.net/jdk/pull/8354
Re: RFR: 8285295: Need better testing for IdentityHashMap [v2]
On Wed, 4 May 2022 15:02:43 GMT, liach wrote:
>> test/jdk/java/util/IdentityHashMap/Basic.java line 500:
>>
>>> 498: Box newKey = new Box(k1a);
>>> 499: Box newVal = new Box(v1a);
>>> 500: Box r = map.computeIfAbsent(newKey, k -> { called[0] = true;
>>> return newVal; });
>>
>> More of a curiosity than a review comment - I see that various places in
>> this PR use a boolean array with one element instead of just a boolean type.
>> Is that a personal coding preference or is there something more to it?
>
> This just serves as a modifiable boolean like an AtomicBoolean. Remember
> lambdas can only use final local var references (due to how they work), and
> it cannot access or modify the local variable in the caller method.
Yes, that's it; you can't mutate a local variable captured by a lambda.
-
PR: https://git.openjdk.java.net/jdk/pull/8354
Re: RFR: 8285295: Need better testing for IdentityHashMap [v2]
On Fri, 29 Apr 2022 03:00:40 GMT, Stuart Marks wrote:
>> Basic but fairly comprehensive set of tests for `IdentityHashMap`. The patch
>> in the bug report that breaks `IdentityHashMap` now causes several cases in
>> this new test to fail. There's more that could be done, but the new tests
>> cover most of the core functions of `IdentityHashMap`. Unfortunately it
>> seems difficult to merge this with the existing, comprehensive Collections
>> tests (e.g., MOAT.java) because those tests implicity rely on
>> `equals()`-based contract instead of the special-purpose `==`-based contract
>> used by `IdentityHashMap`.
>
> Stuart Marks has updated the pull request incrementally with two additional
> commits since the last revision:
>
> - Assertions over return values. Some refinement of equals() testing.
> - Add comment about Map.Entry identity not guaranteed.
Hello Stuart, I had a look at the updates. This looks good to me, thank you for
the changes. Just a couple of comments/questions that I've included at relevant
lines.
test/jdk/java/util/IdentityHashMap/Basic.java line 50:
> 48: // that a map's entrySet contains exactly the expected mappings. In most
> cases, keys and
> 49: // values should be compared by identity, not equality. However, the
> identities of Map.Entry
> 50: // instances from an IdentityHashSet are not guaranteed; the keys and
> values they contain
I think I understand what you are saying here, but it took me a few reads to
understand it. More so because of `IdentityHashSet` here, which I think is a
typo.
So what's being stated here is that you cannot do something like:
IdentityHashMap m = new IdentityHashMap();
...
var e1 = m.entrySet();
var e2 = m.entrySet();
assertSame(e1, e2); // this isn't guaranteed to pass
Did I understand this right?
test/jdk/java/util/IdentityHashMap/Basic.java line 500:
> 498: Box newKey = new Box(k1a);
> 499: Box newVal = new Box(v1a);
> 500: Box r = map.computeIfAbsent(newKey, k -> { called[0] = true;
> return newVal; });
More of a curiosity than a review comment - I see that various places in this
PR use a boolean array with one element instead of just a boolean type. Is that
a personal coding preference or is there something more to it?
-
PR: https://git.openjdk.java.net/jdk/pull/8354
Re: RFR: 8285295: Need better testing for IdentityHashMap [v2]
On Wed, 4 May 2022 14:57:19 GMT, Jaikiran Pai wrote:
>> Stuart Marks has updated the pull request incrementally with two additional
>> commits since the last revision:
>>
>> - Assertions over return values. Some refinement of equals() testing.
>> - Add comment about Map.Entry identity not guaranteed.
>
> test/jdk/java/util/IdentityHashMap/Basic.java line 500:
>
>> 498: Box newKey = new Box(k1a);
>> 499: Box newVal = new Box(v1a);
>> 500: Box r = map.computeIfAbsent(newKey, k -> { called[0] = true;
>> return newVal; });
>
> More of a curiosity than a review comment - I see that various places in this
> PR use a boolean array with one element instead of just a boolean type. Is
> that a personal coding preference or is there something more to it?
This just serves as a modifiable boolean like an AtomicBoolean. Remember
lambdas can only use final local var references (due to how they work), and it
cannot access or modify the local variable in the caller method.
-
PR: https://git.openjdk.java.net/jdk/pull/8354
Re: RFR: 8285295: Need better testing for IdentityHashMap [v2]
On Fri, 29 Apr 2022 03:00:40 GMT, Stuart Marks wrote: >> Basic but fairly comprehensive set of tests for `IdentityHashMap`. The patch >> in the bug report that breaks `IdentityHashMap` now causes several cases in >> this new test to fail. There's more that could be done, but the new tests >> cover most of the core functions of `IdentityHashMap`. Unfortunately it >> seems difficult to merge this with the existing, comprehensive Collections >> tests (e.g., MOAT.java) because those tests implicity rely on >> `equals()`-based contract instead of the special-purpose `==`-based contract >> used by `IdentityHashMap`. > > Stuart Marks has updated the pull request incrementally with two additional > commits since the last revision: > > - Assertions over return values. Some refinement of equals() testing. > - Add comment about Map.Entry identity not guaranteed. I've added checking of return values for I think everything that has a significant return value. I've elected to store the return value in a local variable and add a separate assert line. For example, instead of assertNull(map.put(newKey, newVal)); I've done Box r = map.put(newKey, newVal); assertNull(r); The reason is that I think it separates the test setup/action from the test assertions. I tried it the first way, but it felt like the lack of this separation made things messy. I've also added a couple more tests over `equals` and added more asserts over `equals` to the `keySet` and `entrySet` view sets. (Note, the `values` collection is just a `Collection` and thus doesn't have a defined notion of `equals`.) The testing of the view collections could probably be made more comprehensive, but I think what's here is a good start. Please take a look. - PR: https://git.openjdk.java.net/jdk/pull/8354
Re: RFR: 8285295: Need better testing for IdentityHashMap [v2]
> Basic but fairly comprehensive set of tests for `IdentityHashMap`. The patch > in the bug report that breaks `IdentityHashMap` now causes several cases in > this new test to fail. There's more that could be done, but the new tests > cover most of the core functions of `IdentityHashMap`. Unfortunately it seems > difficult to merge this with the existing, comprehensive Collections tests > (e.g., MOAT.java) because those tests implicity rely on `equals()`-based > contract instead of the special-purpose `==`-based contract used by > `IdentityHashMap`. Stuart Marks has updated the pull request incrementally with two additional commits since the last revision: - Assertions over return values. Some refinement of equals() testing. - Add comment about Map.Entry identity not guaranteed. - Changes: - all: https://git.openjdk.java.net/jdk/pull/8354/files - new: https://git.openjdk.java.net/jdk/pull/8354/files/5857fe6f..d66ad0b8 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=8354&range=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=8354&range=00-01 Stats: 117 lines in 1 file changed: 84 ins; 2 del; 31 mod Patch: https://git.openjdk.java.net/jdk/pull/8354.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8354/head:pull/8354 PR: https://git.openjdk.java.net/jdk/pull/8354
