[WIP] OccupiedEntry.replace_

[Michael-J-Ward]

First pass at implementing the OccupiedEntry.replace_ in #4

This is directly taken from hashbrown.

I see the existing warning in src/map.rs that the hashbrown implementation will not work (

griddle/src/map.rs

Lines 1857 to 1865 in 13509e4

	// NOTE:
	// replace_entry and replace_key are _not_ okay, because it may move a bucket in the old
	// table to _before_ where the "move keys iterator" is currently pointed. in which case we'd
	// fail to move them.
	//
	// it should be possible to work around this by checking if that's the case, and then
	// restarting the iterator, though that would come at a performance penalty...
	//
	// when implemented, remove the #[allow(dead_code)] on OccupiedEntry::key

)

However, I do not see where the bucket gets moved. Aren't the replace_entry and replace_key replacing the values in place?

I additionally ran a test with a million random inserts / replaces comparing the results of griddle::HashMap vs hashbrown::HashMap and it passed.

Do you have any guidance on better understanding / triggering the warning condition?

Thanks

[jonhoo]

Hmm, that's interesting. It feels like there has to be an unstated requirement that the new key hashes to the same as the old one for this to work. Apparently these methods are still under discussion in rust-lang/rust#44286, so I filed a comment there: rust-lang/rust#44286 (comment). Since this just seems to work though, and doesn't require shifting, I'm inclined to accept it!

One thing though: there are probably tests for replace_ in map as well that we should move over.

[Michael-J-Ward]

My strategy for that was to place assert_eq!(1, 2) at the first line of each method in hashbrown, run the hashbrown tests with --no-fail-fast, and see what fails.

Only the test_set::test_replace unit test fails:

failures:
    set::test_set::test_replace

test result: FAILED. 62 passed; 1 failed; 0 ignored; 0 measured; 0 filtered out

And these doctests

failures:
    src/map.rs - map::OccupiedEntry::replace_entry (line 2567)
    src/set.rs - set::HashSet<T, S>::replace (line 845)

test result: FAILED. 96 passed; 2 failed; 0 ignored; 0 measured; 0 filtered out

[jonhoo]

Oh, how weird... I definitely feel like we should have more tests for this, but it sounds like we should contribute those tests to hashbrown first then.

I just realized why this is okay (rust-lang/rust#44286 (comment)), and so I think it's fine for us to include this too then! Thank you :)

[Michael-J-Ward]

Should that warning note about the replace_ implementations also be removed?

[jonhoo]

Ah, yes, good catch! I just pushed that change.