Fix PrimitiveHashSet#iterator()#remove() to not rehash. Fixes #481 #484
Conversation
…#481 Signed-off-by: Nikhil Nanivadekar <nikhil.nanivadekar@gs.com>
|
LGTM 👍 |
|
I will create service releases for 7.x and 8.x. The service releases can be pushed anytime, so we can push them sooner than April 18. |
| @@ -1610,7 +1615,7 @@ public class <name>HashSet extends Abstract<name>Set implements Mutable<name>Set | |||
| { | |||
| removeValue = <name>HashSet.this.table[this.position - 1]; | |||
| } | |||
| <name>HashSet.this.remove(removeValue); | |||
| <name>HashSet.this.remove(removeValue, false); | |||
There was a problem hiding this comment.
While this is good enough to fix the bug, I think there's a better implementation that we should add to the backlog.
We should basically inline the call to remove here, removing the call to rehash, and directly mutating the backing table. When we delegate to remove(), we'll be calling probe() again. We already know the position in the table, because we're iterating through positions and have the position field.
This would also be more consistent with UnifiedSet.
This MR doesn't seem to address the same bug in primitive maps, where they delegate to removeKey and rehash.
There was a problem hiding this comment.
I don't see a rehash in the maps. I checked 7.x, 8.x and 9.x
|
Looking now, maps never seem to rehash during removal. Perhaps that was my
source of confusion. So no bug, which is good, though surprisingly
inconsistent with sets.
…On Mon, Mar 19, 2018 at 12:08 PM Mohammad Rezaei ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In
eclipse-collections-code-generator/src/main/resources/impl/set/mutable/primitiveHashSet.stg
<#484 (comment)>
:
> @@ -1610,7 +1615,7 @@ public class <name>HashSet extends Abstract<name>Set implements Mutable<name>Set
{
removeValue = <name>HashSet.this.table[this.position - 1];
}
- <name>HashSet.this.remove(removeValue);
+ <name>HashSet.this.remove(removeValue, false);
I don't see a rehash in the maps. I checked 7.x, 8.x and 9.x
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
<#484 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAO6Ih_tsw5sU_FblSrJQ00Xd-nfwiO0ks5tf9gEgaJpZM4SvhbS>
.
|
|
Last night I had verified all Maps, Sets and Bags. Only primitiveSets has this issue. |
No description provided.