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

Allow comparing Vecs with different allocators using == #93755

Conversation

ChayimFriedman2
Copy link
Contributor

@ChayimFriedman2 ChayimFriedman2 commented Feb 8, 2022

See https://stackoverflow.com/q/71021633/7884305.

I did not changed the PartialOrd impl too because it was not generic already (didn't support Vec<T> <=> Vec<U> where T: PartialOrd<U>).

Does it needs tests?

I don't think this will hurt type inference much because the default allocator is usually not inferred (new() specifies it directly, and even with other allocators, you pass the allocator to new_in() so the compiler usually knows the type).

I think this requires FCP since the impls are already stable.

@rust-highfive
Copy link
Collaborator

r? @joshtriplett

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Feb 8, 2022
@dtolnay dtolnay added T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. needs-fcp This change is insta-stable, so needs a completed FCP to proceed. labels Mar 18, 2022
@dtolnay dtolnay assigned dtolnay and unassigned joshtriplett Mar 18, 2022
@dtolnay
Copy link
Member

dtolnay commented Mar 18, 2022

@rust-lang/libs-api:
@rfcbot fcp merge

Before:

impl<T, U, A> PartialEq<Vec<U, A>> for Vec<T, A>
where
    T: PartialEq<U>,
    A: Allocator;

After:

impl<T, U, A1, A2> PartialEq<Vec<U, A2>> for Vec<T, A1>
where
    T: PartialEq<U>,
    A1: Allocator,
    A2: Allocator;

@rfcbot
Copy link

rfcbot commented Mar 18, 2022

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

No concerns currently listed.

Once a majority of reviewers approve (and at most 2 approvals are outstanding), 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.

@rfcbot rfcbot added proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. labels Mar 18, 2022
@rfcbot
Copy link

rfcbot commented Mar 19, 2022

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

@rfcbot rfcbot added final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. and removed proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. labels Mar 19, 2022
@joshtriplett
Copy link
Member

We should watch for any issues with this in the beta crater runs. I don't anticipate an issue either.

@dtolnay
Copy link
Member

dtolnay commented Mar 28, 2022

@bors r+

@bors
Copy link
Contributor

bors commented Mar 28, 2022

📌 Commit ee23fd2 has been approved by dtolnay

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Mar 28, 2022
Dylan-DPC added a commit to Dylan-DPC/rust that referenced this pull request Mar 28, 2022
…s-with-different-allocators, r=dtolnay

Allow comparing `Vec`s with different allocators using `==`

See https://stackoverflow.com/q/71021633/7884305.

I did not changed the `PartialOrd` impl too because it was not generic already (didn't support `Vec<T> <=> Vec<U> where T: PartialOrd<U>`).

Does it needs tests?

I don't think this will hurt type inference much because the default allocator is usually not inferred (`new()` specifies it directly, and even with other allocators, you pass the allocator to `new_in()` so the compiler usually knows the type).

I think this requires FCP since the impls are already stable.
This was referenced Mar 28, 2022
bors added a commit to rust-lang-ci/rust that referenced this pull request Mar 28, 2022
Rollup of 4 pull requests

Successful merges:

 - rust-lang#88375 (Clarify that ManuallyDrop<T> has same layout as T)
 - rust-lang#93755 (Allow comparing `Vec`s with different allocators using `==`)
 - rust-lang#95016 (Docs: make Vec::from_raw_parts documentation less strict)
 - rust-lang#95098 (impl From<&[T; N]> and From<&mut [T; N]> for Vec<T>)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors
Copy link
Contributor

bors commented Mar 28, 2022

⌛ Testing commit ee23fd2 with merge 93313d1...

@bors bors merged commit 6ed1a67 into rust-lang:master Mar 28, 2022
@rustbot rustbot added this to the 1.61.0 milestone Mar 28, 2022
@ChayimFriedman2 ChayimFriedman2 deleted the allow-comparing-vecs-with-different-allocators branch March 28, 2022 05:15
@rfcbot rfcbot added finished-final-comment-period The final comment period is finished for this PR / Issue. to-announce Announce this issue on triage meeting and removed final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. labels Mar 29, 2022
@apiraino apiraino removed the to-announce Announce this issue on triage meeting label Mar 31, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. finished-final-comment-period The final comment period is finished for this PR / Issue. needs-fcp This change is insta-stable, so needs a completed FCP to proceed. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants