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

Some improvements in collections #14279

Merged
merged 5 commits into from
May 19, 2014
Merged

Some improvements in collections #14279

merged 5 commits into from
May 19, 2014

Conversation

aochagavia
Copy link
Contributor

The breaking changes are:

  • Changed DList::insert_ordered to use TotalOrd, not Ord
  • Changed PriorityQueue to use TotalOrd, not Ord
  • Deprecated PriorityQueue::maybe_top() (renamed to replace PriorityQueue::top())
  • Deprecated PriorityQueue::maybe_pop() (renamed to replace PriorityQueue::pop())
  • Deprecated PriorityQueue::to_vec() (renamed to PriorityQueue::into_vec())
  • Deprecated PriorityQueue::to_sorted_vec() (renamed to PriorityQueue::into_sorted_vec())
  • Changed PriorityQueue::replace(...) to return an Option<T> instead of failing when the queue is empty.

[breaking-change]

@huonw
Copy link
Member

huonw commented May 18, 2014

Thanks for doing this.

While you're doing modifying PriorityQueue, could you rename to_vec and to_sorted_vec to match the conventions? The correct procedure would be to deprecate them:

    #[deprecated="renamed to `into_vec`"]
    fn to_vec(self) -> Vec<T> { self.into_vec() }

    #[deprecated="renamed to `into_sorted_vec`"]
    fn to_sorted_vec(self) -> Vec<T> { self.into_sorted_vec() }

    fn into_vec(self) { /* current code in `.to_vec` */ }
    fn into_sorted_vec(self) { /* current code in `.to_sorted_vec` */ }

Also, the Ord commit and that renaming should both be marked with [breaking-change] to match our new changes procedure.

fn from_iter<Iter: Iterator<T>>(iter: Iter) -> PriorityQueue<T> {
let mut q = PriorityQueue::new();
let (lower, _) = iter.size_hint();
Copy link
Member

Choose a reason for hiding this comment

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

This isn't actually necessary: it is essentially free (no allocation) to create an empty priority queue, and .extend (below) reserves space itself.

@aochagavia
Copy link
Contributor Author

I will add that.

Also, I think we should get rid of pop() (which fails if the queue is empty) and rename maybe_pop() to pop(). The same for top(). What do you think about it?

@huonw
Copy link
Member

huonw commented May 18, 2014

Yep, sounds good.

@aochagavia
Copy link
Contributor Author

I think it is ready! I have done what you said and also added TotalOrd to DList::insert_ordered

@huonw r?

@aochagavia aochagavia changed the title Some improvements in collections::PriorityQueue Some improvements in collections May 18, 2014
@huonw
Copy link
Member

huonw commented May 19, 2014

The commits that deprecate or change signatures need to have the [breaking-change] annotation as described in: https://mail.mozilla.org/pipermail/rust-dev/2014-April/009543.html

Or, alternatively, it's OK if you edited the PR description to include a summary of all the changes, and put the [breaking-change] annotation .

@aochagavia
Copy link
Contributor Author

@huonw Done!

bors added a commit that referenced this pull request May 19, 2014
The breaking changes are:

* Changed `DList::insert_ordered` to use `TotalOrd`, not `Ord`
* Changed `PriorityQueue` to use `TotalOrd`, not `Ord`
* Deprecated `PriorityQueue::maybe_top()` (renamed to replace `PriorityQueue::top()`)
* Deprecated `PriorityQueue::maybe_pop()` (renamed to replace `PriorityQueue::pop()`)
* Deprecated `PriorityQueue::to_vec()` (renamed to `PriorityQueue::into_vec()`)
* Deprecated `PriorityQueue::to_sorted_vec()` (renamed to `PriorityQueue::into_sorted_vec()`)
* Changed `PriorityQueue::replace(...)` to return an `Option<T>` instead of failing when the queue is empty.


[breaking-change]
@bors bors closed this May 19, 2014
@bors bors merged commit 3a1b7d4 into rust-lang:master May 19, 2014
@aochagavia aochagavia deleted the pr2 branch May 28, 2014 14:22
bors added a commit to rust-lang-ci/rust that referenced this pull request Jun 5, 2023
…albasi

fix: ignore impls with `#[rustc_reservation_impl]`

Fixes rust-lang#12247
Fixes rust-lang#14279

Currently core has two blanket impls for `From`: `impl<T> From<T> for T` and `impl<T> From<!> for T`. These are conflicting and thus chalk cannot uniquely solve `S: From<?0>` for any type `S`.

The latter impl is actually a reservation impl and should not be considered during trait selection. More generally, impls attributed with perma-unstable `#[rustc_reservation_impl]` attribute should be disregarded except for coherence checks. See rust-lang#64631 and rust-lang#64715 for details.

I chose to entirely ignore them in hir-ty because we don't do coherence checks.
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.

3 participants