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

Implement compute_if_present #32

Merged
merged 17 commits into from
Jan 30, 2020
Merged

Implement compute_if_present #32

merged 17 commits into from
Jan 30, 2020

Conversation

voidc
Copy link
Contributor

@voidc voidc commented Jan 25, 2020

This pull request contains the compute_if_present method, which is the simplest one among the reservation-based methods mentioned in #12 since it does not require ReservationNodes.

I took most of the code from put and remove.
For now, I only added basic tests but I also want to port some of the Java tests.

src/map.rs Outdated Show resolved Hide resolved
src/map.rs Show resolved Hide resolved
src/map.rs Outdated Show resolved Hide resolved
src/map.rs Outdated Show resolved Hide resolved
@jonhoo
Copy link
Owner

jonhoo commented Jan 25, 2020

This looks quite good! I left a couple of inline notes.

It makes me want to try to figure out how we might be able to unify the code for put, compute_if_present, remove, and maybe even some others. They all have roughly the same "outer" code.. But that's for another time.

@jonhoo
Copy link
Owner

jonhoo commented Jan 25, 2020

@voidc Hmm, why do you want the semantics to be that it returns the new value rather than the old? Or, I guess, it could return both?

@voidc
Copy link
Contributor Author

voidc commented Jan 25, 2020

Hi! Thanks for the comments!
I changed the function to return the computed, new value so it matches the Java version.
But your are right, it could return both.

@jonhoo
Copy link
Owner

jonhoo commented Jan 25, 2020

Ohh, I see, I missed that! Yes, I think returning references to both old and new would be good.

src/map.rs Outdated Show resolved Hide resolved
@domenicquirl
Copy link
Collaborator

@voidc Hi, I just submitted #35 to deal with #9 and noticed you are also matching against BinEntry::Moved and have copied over the corresponding safety comment. Would you mind to change the comment to reference the new safety statement (it's now on the definition of Moved) so we don't spread the FIXME further? 😊

@jonhoo jonhoo marked this pull request as ready for review January 30, 2020 00:30
@jonhoo
Copy link
Owner

jonhoo commented Jan 30, 2020

@voidc Unless there's more you want to do with this, I think I'm happy to merge once you bring it up to date with master!

@codecov
Copy link

codecov bot commented Jan 30, 2020

Codecov Report

Merging #32 into master will increase coverage by 1.55%.
The diff coverage is 96.29%.

Impacted Files Coverage Δ
src/iter/iter.rs 92.5% <ø> (ø) ⬆️
src/node.rs 45.45% <100%> (ø) ⬆️
src/map.rs 88.76% <96.15%> (+2.14%) ⬆️

@voidc
Copy link
Contributor Author

voidc commented Jan 30, 2020

Thanks for the approval! Feel free to merge.
Points of (future) consideration:

  • Also return the old value (although this would mean being inconsistent with the other compute_* methods as they don't necessarily have an old value)
  • Possibly unify code with replace_node and put.

@jonhoo jonhoo merged commit 82111cf into jonhoo:master Jan 30, 2020
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.

3 participants