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

Common traits PartialEq, Eq and Hash #74

Closed
wants to merge 9 commits into from

Conversation

Projects
None yet
2 participants
@dns2utf8
Copy link
Member

dns2utf8 commented Jul 15, 2017

I implemented PartialEq, Eq and Hash. Others like Copy or PartialOrd did not seem right.

@dns2utf8

This comment has been minimized.

Copy link
Member Author

dns2utf8 commented Jul 15, 2017

Currently the build fails prior to rustc 1.12.0 because of the assert_ne! macro.
The DefaultHasher requires rustc 1.13.0.

Should we work around it or update the min version? Debian 9 stable currently serves rust 1.14.0 with the default installation.

@frewsxcv

This comment has been minimized.

Copy link
Collaborator

frewsxcv commented Jul 26, 2017

PartialEq and Eq make a bit of sense to me. Not sure why you'd need to compare threadpools in the real world, but sure, I think this is fine. I'm a little confused by Hash though. Are you aware of any real world examples involving Hash? Just trying to better understand why the trait implementation is being added

@dns2utf8 dns2utf8 force-pushed the dns2utf8:common_traits branch from 0166379 to aeec3d7 Jul 28, 2017

@dns2utf8 dns2utf8 force-pushed the dns2utf8:common_traits branch from db6eaa3 to b9ff22f Jul 28, 2017

@dns2utf8

This comment has been minimized.

Copy link
Member Author

dns2utf8 commented Jul 28, 2017

My intention was to implement everything I thought reasonable.
I made a test case with the hash set. I agree do not see an imminent real-world use-case for the hash trait yet.

I implemented it from the list of this document referenced here:
Types eagerly implement common traits ([C-COMMON-TRAITS])
- Copy, Clone, Eq, PartialEq, Ord, PartialOrd, Hash, Debug, Display, Default

We could also split this PR and only merge the PartialEq and Eq.

@frewsxcv frewsxcv referenced this pull request Aug 11, 2017

Merged

PartialEq/Eq #81

@frewsxcv

This comment has been minimized.

Copy link
Collaborator

frewsxcv commented Aug 11, 2017

cherry-picked your PartialEq/Eq impls from this PR and merged them in #81. thanks!

@frewsxcv frewsxcv closed this Aug 11, 2017

@dns2utf8

This comment has been minimized.

Copy link
Member Author

dns2utf8 commented Aug 11, 2017

Cool, thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.