Skip to content

Conversation

cuviper
Copy link
Member

@cuviper cuviper commented Jun 29, 2017

We can't use a derived Hash when we have a manual Eq, because we
need to uphold the invariant a == bh(a) == h(b). Since Eq
doesn't require them to be in reduced form, Hash also needs to be
normalized.

Fixes #310.

@cuviper
Copy link
Member Author

cuviper commented Jun 29, 2017

Technically this is a breaking change, because the former derived Hash only required T: Hash, and here I've used T: Clone + Integer + Hash. However, every other operation on Ratio<T> requires Clone + Integer, and we can't fix the relationship to Eq with just T: Hash.

(I don't actually need Clone here, but I think it's better to be consistent.)

We can't use a derived `Hash` when we have a manual `Eq`, because we
need to uphold the invariant `a == b` → `h(a) == h(b)`.  Since `Eq`
doesn't require them to be in reduced form, `Hash` also needs to be
normalized.
@cuviper
Copy link
Member Author

cuviper commented Jul 10, 2017

There's not even a way to create a Ratio<T> without T: Clone + Integer except through deserialization. I think the only practical way this will break is for a generic function that requires only T: Hash but expects to use Ratio<T> as Hash from that. I think this is outweighed by the need to fix Hash related to Eq.

Thanks to @mbrubeck for reviewing this with me on IRC.

bors r+

bors bot added a commit that referenced this pull request Jul 10, 2017
311: rational: make sure Hash agrees with Eq r=cuviper

We can't use a derived `Hash` when we have a manual `Eq`, because we
need to uphold the invariant `a == b` → `h(a) == h(b)`.  Since `Eq`
doesn't require them to be in reduced form, `Hash` also needs to be
normalized.

Fixes #310.
@bors
Copy link
Contributor

bors bot commented Jul 10, 2017

Build succeeded

@bors bors bot merged commit 3f32ad4 into rust-num:master Jul 10, 2017
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.

1 participant