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

Fix replace missing value #246

Closed
wants to merge 2 commits into from

Conversation

fvlad
Copy link

@fvlad fvlad commented May 28, 2019

Fix to the issue #244 to avoid throwing AssertionError when value is missing by key on replace

@coveralls
Copy link

coveralls commented May 28, 2019

Pull Request Test Coverage Report for Build 533

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 47.595%

Totals Coverage Status
Change from base Build 529: 0.0%
Covered Lines: 1672
Relevant Lines: 3513

💛 - Coveralls

@nitsanw nitsanw self-requested a review May 28, 2019 11:45
@nitsanw
Copy link
Contributor

nitsanw commented May 28, 2019

Javadoc for replace:
"replace(K key, V value): Replaces the entry for the specified key only if it is currently mapped to some value."
So, we expect a null reply, and the assert in putIfMatch seems to contradict the contract.

The code in putIfMatch has an assert that is stronger than the guarantees in the static putIfMatch, but that assert is valid for all case where the oldVal param is not MATCH_ANY. I think the right move here is to add this observation to the assert:

assert res != null || oldVal == MATCH_ANY;

Copy link
Contributor

@nitsanw nitsanw left a comment

Choose a reason for hiding this comment

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

See other comment

@nitsanw
Copy link
Contributor

nitsanw commented May 28, 2019

Further digging after #246 (comment) leads me to think that putIfMatch should have returned TOMBSTONE instead of null here.

@nitsanw
Copy link
Contributor

nitsanw commented May 28, 2019

Please see changes in 1475be5 which resolve the issue. I'm very grateful for the PR and would happily merge it if you revert the change to NBHM. Thanks!

@fvlad
Copy link
Author

fvlad commented May 28, 2019

Great, thanks for your fix!

@fvlad fvlad closed this May 28, 2019
@fvlad fvlad deleted the replace-missing-value-fix branch May 28, 2019 15:55
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.

None yet

4 participants