Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

derive Hash (and not Copy) for ranges #34180

Merged
merged 4 commits into from
Jun 15, 2016
Merged

derive Hash (and not Copy) for ranges #34180

merged 4 commits into from
Jun 15, 2016

Conversation

durka
Copy link
Contributor

@durka durka commented Jun 9, 2016

Fixes #34170.

Also, RangeInclusive was Copy by mistake -- fix that, which is a [breaking-change] to that unstable type.

@rust-highfive
Copy link
Collaborator

r? @brson

(rust_highfive has picked a reviewer for you, use r? to override)

@durka
Copy link
Contributor Author

durka commented Jun 9, 2016

r? @sfackler

@rust-highfive rust-highfive assigned sfackler and unassigned brson Jun 9, 2016
@hayd
Copy link

hayd commented Jun 9, 2016

This could do with some tests.

@ollie27
Copy link
Member

ollie27 commented Jun 9, 2016

Is there a reason you've not included RangeInclusive?

@durka
Copy link
Contributor Author

durka commented Jun 9, 2016

@ollie27 no, it was a mistake.

On Thu, Jun 9, 2016 at 1:12 PM, Oliver Middleton notifications@github.com
wrote:

Is there a reason you've not included RangeInclusive?


You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
#34180 (comment), or mute
the thread
https://github.com/notifications/unsubscribe/AAC3n0Z9hD6j25O1SYPXrCC7VcnLFBOMks5qKEmBgaJpZM4IyD3K
.

@durka
Copy link
Contributor Author

durka commented Jun 9, 2016

And RangeInclusive is Copy! Oops!

[breaking-change] due to the removal of Copy which shouldn't have been there in the first place, as per policy set forth in rust-lang#27186.
@durka
Copy link
Contributor Author

durka commented Jun 9, 2016

Tests are annoying because rustc won't continue past any error about Copy. So I guess I'll have to create seven separate cfail tests.

@durka
Copy link
Contributor Author

durka commented Jun 9, 2016

Tests added.

@durka durka changed the title derive Hash for ranges derive Hash (and not Copy) for ranges Jun 9, 2016
//~^^^^^^^^ ERROR binary operation
//~^^^^^^^^^ ERROR binary operation
//~^^^^^^^^^^ ERROR binary operation
//~^^^^^^^^^^^ ERROR binary operation
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Why are these errors duplicated so many times?

Copy link
Contributor

Choose a reason for hiding this comment

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

Can you file an issue about the bogus duplicate errors and add a FIXME here linking it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will do.
On Jun 10, 2016 8:27 PM, "Brian Anderson" notifications@github.com wrote:

In src/test/compile-fail/range_traits-1.rs
#34180 (comment):

+use std::ops::*;
+
+#[derive(Clone, PartialEq, Eq, PartialOrd, Ord, Hash, Debug)]
+struct AllTheRanges {

  • a: Range,
  • //~^ ERROR PartialOrd
  • //~^^ ERROR PartialOrd
  • //~^^^ ERROR Ord
  • //~^^^^ ERROR binary operation
  • //~^^^^^ ERROR binary operation
  • //~^^^^^^ ERROR binary operation
  • //~^^^^^^^ ERROR binary operation
  • //~^^^^^^^^ ERROR binary operation
  • //~^^^^^^^^^ ERROR binary operation
  • //~^^^^^^^^^^ ERROR binary operation
  • //~^^^^^^^^^^^ ERROR binary operation

Can you file an issue about the bogus duplicate errors and add a FIXME
here linking it?


You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
https://github.com/rust-lang/rust/pull/34180/files/53618c36292955b0c79e9ebc02df879015813851#r66696919,
or mute the thread
https://github.com/notifications/unsubscribe/AAC3n2zMYixNPrAmRjPg75o2A7BBa3b9ks5qKgEIgaJpZM4IyD3K
.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@durka
Copy link
Contributor Author

durka commented Jun 11, 2016

@brson done.

@brson
Copy link
Contributor

brson commented Jun 15, 2016

@bors r+

@bors
Copy link
Contributor

bors commented Jun 15, 2016

📌 Commit df924ca has been approved by brson

@bors
Copy link
Contributor

bors commented Jun 15, 2016

⌛ Testing commit df924ca with merge a948815...

bors added a commit that referenced this pull request Jun 15, 2016
derive Hash (and not Copy) for ranges

Fixes #34170.

Also, `RangeInclusive` was `Copy` by mistake -- fix that, which is a [breaking-change] to that unstable type.
@bors bors merged commit df924ca into rust-lang:master Jun 15, 2016
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

std::ops::Range does not implement std::hash::Hash
7 participants