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

Add PartialEq, From<T> derives + Fix rustc 1.24.1 builds #7

Merged
merged 4 commits into from
Jan 14, 2019

Conversation

mmstick
Copy link
Contributor

@mmstick mmstick commented Jan 11, 2019

I'd like to use this within System76 / Pop!_OS projects, but that requires an extra derive (PartialEq), and using use import syntax that's compatible with rustc 1.24.1. While I was there, added some additional missing derives (Hash, From).

@matklad
Copy link
Owner

matklad commented Jan 11, 2019

Hm, I think we defenetely can’t implement Hash: otherwise, you can store an empty cell in a hash map, then fill it, and get a hash map in an inconsistent state.

It can be said that Eq and Ord could screw data structures in a similar way, but I think I’d be fine with those. Cc
rust-lang/rust#33306

@mmstick
Copy link
Contributor Author

mmstick commented Jan 11, 2019

I've removed the Hash derives.

self.get() == other.get()
}
}

Copy link
Owner

Choose a reason for hiding this comment

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

This probably needs to be duplicated in impl_std?

And perhaps it makes sense to add a trivial test for this API as well, exactly to test that both impls in fact have the same api?

@mmstick mmstick changed the title Add PartialEq, Hash, From<T> derives + Fix rustc 1.24.1 builds Add PartialEq, From<T> derives + Fix rustc 1.24.1 builds Jan 11, 2019
@matklad
Copy link
Owner

matklad commented Jan 14, 2019

Awesome, thanks!

@matklad matklad merged commit b4e99bc into matklad:master Jan 14, 2019
@matklad
Copy link
Owner

matklad commented Jan 14, 2019

released as 0.1.7

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.

2 participants