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 From<HashMap<T, ()>> for HashSet<T>. #235

Merged
merged 1 commit into from
Feb 5, 2021
Merged

Implement From<HashMap<T, ()>> for HashSet<T>. #235

merged 1 commit into from
Feb 5, 2021

Conversation

smmalis37
Copy link
Contributor

Closes #219.

@smmalis37
Copy link
Contributor Author

Those clippy CI failures don't look related to this?

@Amanieu
Copy link
Member

Amanieu commented Feb 4, 2021

This happens when clippy adds a new lint. Can you add the # Panics section to the doc comments that need it? You can do it either in this PR or in a separate one.

@smmalis37
Copy link
Contributor Author

smmalis37 commented Feb 4, 2021

I could if I understood why these functions can panic. I attempted to dig into the codebase, but I can't understand why

key: None,
sets key to None. As far as I can see this is the only way to get a key of None, which will cause these panics later. Example code that panics:

let mut m: HashMap<i32, i32> = HashMap::new();
let o = m.entry(0).insert(1);
let (k, v) = o.replace_entry(2);

The same behavior should be possible with replace_key instead of replace_entry as well.

Why does OccupiedEntry have key as an Option? Could it be changed to always be present? If it can't, can these two unwraps be changed to handle this case better?

@Amanieu
Copy link
Member

Amanieu commented Feb 4, 2021

key is the key that was used to perform the lookup. It is the value passed to HashMap::entry. However it is None when the OccupiedEntry is created from VacantEntry::insert_entry since the key is then moved into the HashMap to create the entry.

@smmalis37
Copy link
Contributor Author

smmalis37 commented Feb 4, 2021

I see. I suppose I can go add Panics if this OccupiedEntry was created through Entry::insert to both of those functions, but that feels like a really weird and unfortunate restriction to have. I'm completely new to this codebase, so forgive me for asking, but is there really no better option?

@Amanieu
Copy link
Member

Amanieu commented Feb 4, 2021

I don't think there is a better option.

@smmalis37
Copy link
Contributor Author

This should be good to go now I think.

@Amanieu
Copy link
Member

Amanieu commented Feb 5, 2021

@bors r+

@bors
Copy link
Contributor

bors commented Feb 5, 2021

📌 Commit ae8c196 has been approved by Amanieu

@bors
Copy link
Contributor

bors commented Feb 5, 2021

⌛ Testing commit ae8c196 with merge b5b5be8...

@bors
Copy link
Contributor

bors commented Feb 5, 2021

☀️ Test successful - checks-travis
Approved by: Amanieu
Pushing b5b5be8 to master...

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.

Implement From<HashMap<T, ()>> for HashSet<T>
3 participants