Implement `PartialEq<[A]>` for `VecDeque<A>`. #38661

frewsxcv · 2016-12-28T20:17:59Z

rust-highfive · 2016-12-28T20:18:06Z

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @aturon (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

frewsxcv · 2016-12-28T20:21:11Z

src/libcollections/vec_deque.rs

@@ -2158,6 +2158,18 @@ impl<A: PartialEq> PartialEq for VecDeque<A> {
 #[stable(feature = "rust1", since = "1.0.0")]
 impl<A: Eq> Eq for VecDeque<A> {}

+#[stable(feature = "vec-deque-partial-eq-slice", since = "")]


Reminder for future self that since should be filled in.

frewsxcv · 2016-12-28T20:26:15Z

A crater run might be good for this (see this comment for more information).

frewsxcv · 2016-12-28T20:58:18Z

src/libcollections/vec_deque.rs

@@ -2158,6 +2158,18 @@ impl<A: PartialEq> PartialEq for VecDeque<A> {
 #[stable(feature = "rust1", since = "1.0.0")]
 impl<A: Eq> Eq for VecDeque<A> {}

+#[stable(feature = "vec-deque-partial-eq-slice", since = "")]
+impl<'a, A: PartialEq> PartialEq<&'a [A]> for VecDeque<A> {


Is this the correct signature? Should it be PartialEq<[A]>?

I think PartialEq<[A]> should be the primary one. Does that change the utility of it? The type inference problems? (I guess it's the same.)

I can change it to impl<A: PartialEq> PartialEq<[A]> for VecDeque<A> and the non-test code compiles.

But if one does:

let mut d = VecDeque::new(); d.push_back('a'); d.push_back('b'); let v = vec!['a', 'b']; assert!(d == &v);

...which looks perfectly valid to me, this compile error appears:

error[E0277]: the trait bound `collections::VecDeque<char>: std::cmp::PartialEq<&collections::vec::Vec<char>>` is not satisfied --> src/libcollections/../libcollectionstest/vec_deque.rs:623:13 | 623 | assert!(d == &v); | ^^^^^^^ the trait `std::cmp::PartialEq<&collections::vec::Vec<char>>` is not implemented for `collections::VecDeque<char>` | = help: the following implementations were found: = help: <collections::VecDeque<A> as std::cmp::PartialEq> = help: <collections::VecDeque<A> as std::cmp::PartialEq<[A]>>

Looks like one would have to write assert!(d == *&v[..]);, which seems less than ideal.

v[..] must be enough. Still, less than ideal, but not surprising, since it's generic and thus &v can't coerce to anything anyway, it would need an explicit impl for &Vec<_>, would it not, to have d == &v compile?

Addressed in the latest commit.

aturon · 2017-01-04T19:03:22Z

cc @rust-lang/libs, this would be instantly stable.

aturon · 2017-01-04T19:03:54Z

@brson, can you do a crater run?

In general, I'm 👍 on this addition.

sfackler · 2017-01-04T19:09:05Z

LGTM assuming the whole world doesn't break, which I don't imagine it will.

alexcrichton · 2017-01-04T19:21:37Z

@aturon wanna fcp merge this? Otherwise lgtm

aturon · 2017-01-04T20:42:22Z

@rfcbot fcp merge

(We'll wait for crater before actually merging.)

rfcbot · 2017-01-04T20:51:15Z

Team member @aturon has proposed to merge this. The next step is review by the rest of the tagged teams:

No concerns currently listed.

Once these reviewers reach consensus, this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

bluss · 2017-01-06T23:07:13Z

To double check, I thought PartialEq<[T]> would be the natural impl (And then further impls based upon that, possibly), but that doesn't seem to be how Vec does it at all. Vec<A> has PartialEq<X> with X = &[B], &mut [B], [B; 0], &[B; 0] where A: PartialEq<B> (and more array impls).

frewsxcv · 2017-01-07T22:10:01Z

Added more impls in the latest commit.

bors · 2017-01-09T19:42:48Z

☔ The latest upstream changes (presumably #38581) made this pull request unmergeable. Please resolve the merge conflicts.

frewsxcv · 2017-01-09T20:38:29Z

Rebased.

brson · 2017-01-20T22:03:32Z

Yes I can crater.

brson · 2017-01-20T22:10:58Z

$ target/debug/crater-cli.exe custom-build https://github.com/frewsxcv/rust 61313c1426d67db83cc333a4eef146e1df758006
created task for https://github.com/frewsxcv/rust
inspector link: https://tools.taskcluster.net/task-inspector/#y-UmEC_nRROj_6lqoJMPYg

$ target/debug/crater-cli.exe custom-build https://github.com/frewsxcv/rust 5c1472a720243314ac5e659f3d621242eb82f2de
created task for https://github.com/frewsxcv/rust
inspector link: https://tools.taskcluster.net/task-inspector/#KmxJ0FxVT1KPOnKE8Xj6EQ

frewsxcv · 2017-01-21T01:56:52Z

I don't really understand the taskcluster interface. Did everything pass?

rfcbot · 2017-01-23T08:25:36Z

🔔 This is now entering its final comment period, as per the review above. 🔔

psst @aturon, I wasn't able to add the final-comment-period label, please do so.

bluss · 2017-01-23T09:06:27Z

There's no discussion here, just me claiming different things about how I expected it to be and how Vec does it. What Vec does is obviously a mess (So many impls), do we really want to continue that?

With so many impls, it's very hard to remember when exactly a thing like VecDeque is comparable to something else.

brson · 2017-01-23T21:13:08Z

Crater says one false positive.

frewsxcv · 2017-01-27T14:56:27Z

Sooooo is this good to go? Should I fill in since?

aturon · 2017-01-27T16:36:23Z

@frewsxcv Let's do it!

Fixes rust-lang#38625.

frewsxcv · 2017-01-28T14:40:16Z

This should be good to go!

aturon · 2017-01-31T15:55:26Z

@bors: r+

bors · 2017-01-31T15:55:27Z

📌 Commit b081872 has been approved by aturon

bors · 2017-01-31T17:14:13Z

⌛ Testing commit b081872 with merge cdf82c4...

Implement `PartialEq<[A]>` for `VecDeque<A>`. Fixes #38625.

bors · 2017-02-01T02:53:04Z

💔 Test failed - status-travis

frewsxcv · 2017-02-01T02:54:16Z

@bors retry

bors · 2017-02-01T04:34:10Z

⌛ Testing commit b081872 with merge 6f923bf...

bors · 2017-02-01T06:13:03Z

💔 Test failed - status-appveyor

frewsxcv · 2017-02-01T12:15:29Z

@bors retry couldn't fetch cargo

bors · 2017-02-01T12:58:51Z

⌛ Testing commit b081872 with merge 4c357d0...

bors · 2017-02-01T13:40:06Z

💔 Test failed - status-travis

alexcrichton · 2017-02-01T17:17:21Z

@bors: retry

…

On Wed, Feb 1, 2017 at 5:40 AM, bors ***@***.***> wrote: 💔 Test failed - status-travis <https://travis-ci.org/rust-lang/rust/builds/197288644> — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#38661 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAD95GU-Sb8fqkeXk1XAfDWuMp8y1ztcks5rYIs4gaJpZM4LXKey> .

bors · 2017-02-01T22:13:39Z

⌛ Testing commit b081872 with merge 123afbd...

bors · 2017-02-02T00:27:15Z

💔 Test failed - status-appveyor

alexcrichton · 2017-02-02T01:10:22Z

@bors: retry * failed to upload to S3 #39074

…

On Wed, Feb 1, 2017 at 4:27 PM, bors ***@***.***> wrote: 💔 Test failed - status-appveyor <https://ci.appveyor.com/project/rust-lang/rust/build/1.0.1780> — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#38661 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAD95MFGrhQHdzwZHvQ26-OjqCUYNmJyks5rYSLlgaJpZM4LXKey> .

bors · 2017-02-02T02:42:11Z

⌛ Testing commit b081872 with merge 2a6f7e4...

Implement `PartialEq<[A]>` for `VecDeque<A>`. Fixes #38625.

bors · 2017-02-02T05:10:35Z

☀️ Test successful - status-appveyor, status-travis
Approved by: aturon
Pushing 2a6f7e4 to master...

rust-highfive assigned aturon Dec 28, 2016

frewsxcv mentioned this pull request Dec 28, 2016

VecDeque<T> should implement PartialEq with &[T] #38625

Closed

frewsxcv added the T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. label Dec 28, 2016

frewsxcv force-pushed the vec-deque-partial-eq branch from e4224fd to 5e5e1f6 Compare December 28, 2016 20:19

frewsxcv changed the title ~~Implement PartialEq<&[A]> for VecDeque.~~ Implement PartialEq<&[A]> for VecDeque<A>. Dec 28, 2016

frewsxcv commented Dec 28, 2016

View reviewed changes

frewsxcv changed the title ~~Implement PartialEq<&[A]> for VecDeque<A>.~~ Implement PartialEq<[A]> for VecDeque<A>. Dec 29, 2016

frewsxcv force-pushed the vec-deque-partial-eq branch 4 times, most recently from 79566a1 to 5458a1f Compare January 7, 2017 22:09

frewsxcv force-pushed the vec-deque-partial-eq branch from 5458a1f to 61313c1 Compare January 9, 2017 20:38

Implement PartialEq<&[A]> for VecDeque<A>.

b081872

Fixes rust-lang#38625.

frewsxcv force-pushed the vec-deque-partial-eq branch from 61313c1 to b081872 Compare January 28, 2017 14:38

bors added a commit that referenced this pull request Jan 31, 2017

Auto merge of #38661 - frewsxcv:vec-deque-partial-eq, r=aturon

cdf82c4

Implement `PartialEq<[A]>` for `VecDeque<A>`. Fixes #38625.

bors added a commit that referenced this pull request Feb 2, 2017

Auto merge of #38661 - frewsxcv:vec-deque-partial-eq, r=aturon

2a6f7e4

Implement `PartialEq<[A]>` for `VecDeque<A>`. Fixes #38625.

bors merged commit b081872 into rust-lang:master Feb 2, 2017

frewsxcv deleted the vec-deque-partial-eq branch February 15, 2017 02:02

ollie27 mentioned this pull request Mar 2, 2017

Regression in astar-0.1.1, Rust 1.17 #40193

Closed

Implement PartialEq<[A]> for VecDeque<A>. #38661

Implement PartialEq<[A]> for VecDeque<A>. #38661

Conversation

frewsxcv commented Dec 28, 2016

rust-highfive commented Dec 28, 2016

frewsxcv Dec 28, 2016

Choose a reason for hiding this comment

frewsxcv commented Dec 28, 2016

frewsxcv Dec 28, 2016

Choose a reason for hiding this comment

bluss Dec 28, 2016

Choose a reason for hiding this comment

frewsxcv Dec 28, 2016 • edited Loading

Choose a reason for hiding this comment

frewsxcv Dec 28, 2016

Choose a reason for hiding this comment

bluss Dec 28, 2016

Choose a reason for hiding this comment

frewsxcv Dec 29, 2016

Choose a reason for hiding this comment

aturon commented Jan 4, 2017

aturon commented Jan 4, 2017

sfackler commented Jan 4, 2017

alexcrichton commented Jan 4, 2017

aturon commented Jan 4, 2017

rfcbot commented Jan 4, 2017 • edited by brson Loading

bluss commented Jan 6, 2017 • edited Loading

frewsxcv commented Jan 7, 2017

bors commented Jan 9, 2017

frewsxcv commented Jan 9, 2017

brson commented Jan 20, 2017

brson commented Jan 20, 2017

frewsxcv commented Jan 21, 2017

rfcbot commented Jan 23, 2017

bluss commented Jan 23, 2017

brson commented Jan 23, 2017

frewsxcv commented Jan 27, 2017

aturon commented Jan 27, 2017

frewsxcv commented Jan 28, 2017

aturon commented Jan 31, 2017

bors commented Jan 31, 2017

bors commented Jan 31, 2017

bors commented Feb 1, 2017

frewsxcv commented Feb 1, 2017

bors commented Feb 1, 2017

bors commented Feb 1, 2017

frewsxcv commented Feb 1, 2017

bors commented Feb 1, 2017

bors commented Feb 1, 2017

alexcrichton commented Feb 1, 2017 via email

bors commented Feb 1, 2017

bors commented Feb 2, 2017

alexcrichton commented Feb 2, 2017 via email

bors commented Feb 2, 2017

bors commented Feb 2, 2017

Implement `PartialEq<[A]>` for `VecDeque<A>`. #38661

Implement `PartialEq<[A]>` for `VecDeque<A>`. #38661

frewsxcv Dec 28, 2016 •

edited

Loading

rfcbot commented Jan 4, 2017 •

edited by brson

Loading

bluss commented Jan 6, 2017 •

edited

Loading