Skip to content

Early return in the slow path of put if key already exists and no_replacement is true #91

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

Merged
merged 1 commit into from
Aug 28, 2020

Conversation

domenicquirl
Copy link
Collaborator

@domenicquirl domenicquirl commented Aug 28, 2020

Early return with an indication that the requested key already existed and a reference back to the value trying to be inserted.

Fixes #90.

I also took the liberty to add a new issues.rs test file to contain reproducible bugs that should now be fixed, so we will know should they appear again.


This change is Reviewable

@codecov
Copy link

codecov bot commented Aug 28, 2020

Codecov Report

Merging #91 into master will increase coverage by 1.09%.
The diff coverage is 33.33%.

Impacted Files Coverage Δ
src/lib.rs 100.00% <ø> (ø)
src/map.rs 84.17% <33.33%> (+2.23%) ⬆️
src/node.rs 74.27% <0.00%> (-0.40%) ⬇️
src/set.rs 93.93% <0.00%> (-0.10%) ⬇️
src/iter/traverser.rs 92.00% <0.00%> (+0.79%) ⬆️
src/raw/mod.rs 88.34% <0.00%> (+2.91%) ⬆️

@domenicquirl
Copy link
Collaborator Author

Interesting. The failure on the warning screening seems to be because #[deny(intra_doc_link_resolution_failure)] was renamed?

@jonhoo
Copy link
Owner

jonhoo commented Aug 28, 2020

Ah, good catch! And 👍 to adding issues.rs. Maybe call it regressions though?

And yes, the lint has been renamed to broken_intra_doc_links (rust-lang/rust#74926).

@domenicquirl
Copy link
Collaborator Author

Added the suggested name change and also updated the lint. Interestingly, on my machine I get the warning for the lint the other way round now :D
Hopefully it's good now on CI.

@jonhoo
Copy link
Owner

jonhoo commented Aug 28, 2020

Ah, it's probably just because you have a slightly older nightly locally which doesn't have that latest rename PR merged!

@domenicquirl
Copy link
Collaborator Author

If it's only changed on nightly than that'll be it, I'm on stable for most things that don't rely on unstable features 👍

@domenicquirl
Copy link
Collaborator Author

Everything but Miri ran I think, and Miri timed out last time if I remember correctly - you fine with merging?

@jonhoo
Copy link
Owner

jonhoo commented Aug 28, 2020

Hmm, miri timing out isn't great. My guess is that it's because this new regression test runs for so many iterations. Iif the test truly requires this many iterations to reproduce the issue, then just use a different number of iterations for miri. You can use cfg!(miri) for this. Otherwise, just reduce the number of iterations and miri should be fine.

@domenicquirl
Copy link
Collaborator Author

Oh, you think? I thought some other stuff like the stress test were pretty large as well, but seems we already have some miri-dependent tests too. Updating again.

This keeps the original number for non-miri tests, since it runs pretty quickly and the intention is to (hopefully) exercise both the regular and the tree bin path, for which we need some collisions.

@jonhoo
Copy link
Owner

jonhoo commented Aug 28, 2020

I believe the stress tests are already #[cfg(not(miri))], but could be wrong?

@domenicquirl
Copy link
Collaborator Author

They are ignored completely it seems. Other tests have smaller ranges and such. In any case, seems to have been it!

@jonhoo jonhoo merged commit f5322c8 into jonhoo:master Aug 28, 2020
@domenicquirl domenicquirl deleted the fix-90 branch August 28, 2020 13:35
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.

HashMap enters unreachable code in try_insert
2 participants