Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support indexRemove in KTypeVTypeHashMap,KTypeHashSet,KTypeWormSet. #24

Merged
merged 3 commits into from
Jun 5, 2021

Conversation

bruno-roustant
Copy link
Collaborator

New API method KTypeVTypeMap.indexRemove() + implementation in KTypeVTypeHashMap and KTypeVTypeWormMap.

New public method indexRemove() in KTypeHashSet and KTypeWormSet.

  • tests

@bruno-roustant bruno-roustant linked an issue Jun 4, 2021 that may be closed by this pull request
Copy link
Member

@dweiss dweiss left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks Bruno - it looks great. A few minor comments inside. I think there is also a randomized regression test against a java collection somewhere... this could add remove operation now too.

@@ -735,6 +735,27 @@ public void indexInsert(int index, KType key) {
}
}

/**
* Removes a key.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it would be safer to say "removes a key index previously acquired from {{@@link #indexOf}}?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right

assert index <= mask ||
(index == mask + 1 && hasEmptyKey);

VType previousValue = Intrinsics.<VType> cast(values[index]);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is perhaps being overly picky but previous value for the empty key could be just returned without accessing the array... on the other hand - maybe it's good that it doesn't cheat and returns what really was in that slot? Can't decide.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't see how we can get the previous value without accessing the array. Could you explain me?
The remove() method gets the empty key's previous value from the array.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Duh. I'm sorry, I don't think I was thinking clearly. The code is obviously correct.

@@ -188,6 +188,20 @@
*/
public void indexInsert(int index, KType key, VType value);

/**
* Removes a key-value pair.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as above - removes a key-value pair at an index previously acquired from...

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right

@dweiss
Copy link
Member

dweiss commented Jun 4, 2021

That test is called "testAgainstHashMap" -

@dweiss
Copy link
Member

dweiss commented Jun 4, 2021

Would you mind adding a changes entry, Bruno? This way I can just do a click-merge via github - I'm away from the office and without all the resources.

@bruno-roustant bruno-roustant changed the title Add KTypeVTypeHashMap.indexRemove and KTypeHashSet/KTypeWormSet.indexRemove. Support indexRemove in KTypeVTypeHashMap,KTypeHashSet,KTypeWormSet. Jun 5, 2021
@bruno-roustant
Copy link
Collaborator Author

Done. Thanks for the review Dawid.

@dweiss dweiss merged commit 0e18c7d into carrotsearch:master Jun 5, 2021
@bruno-roustant bruno-roustant deleted the indexremove branch June 7, 2021 07:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Does KTypeVTypeMap.indexRemove(int index) miss?
2 participants