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

VecDeque has no sort_by method (should possibly have one to match Vec). #27322

Closed
mitchmindtree opened this issue Jul 27, 2015 · 15 comments
Closed
Labels
A-collections Area: std::collections. C-feature-request Category: A feature request, i.e: not implemented / a PR. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.

Comments

@mitchmindtree
Copy link
Contributor

I just came across a use case for this while working on a visit order for the UI graph in conrod - its absence took me by surprise 😸

Is there a particular reason this hasn't been implemented yet? Or has it just not popped up yet (pun intended)?

@tamird
Copy link
Contributor

tamird commented Jul 27, 2015

How come either method has to be inherent? Seems like this could be a trait that requires sort and provides sort_by.

@abonander
Copy link
Contributor

sort and sort_by are methods provided by [T]. Since the sorting algorithm requires 2 * n extra space, I don't see a too much problem with copying the data from the disjoint slices into one allocation to sort and then putting it back in-order. It's mostly just a question of semantics.

@retep998
Copy link
Member

Wouldn't it have to be sort_by which is required and sort have a default implementation?
I'd prefer if VecDeque had an optimal in place sorting algorithm though.

@Gankra
Copy link
Contributor

Gankra commented Oct 30, 2015

@retep998 the thing is std has no optimal or inplace sorting! All we have is naive merge sort (because it was argued that our sort should be stable). Our sort support is a bit trash imo. Quicksort (unstable) or wikisort (stable but complicated) would be good to have here. Maybe even expose insertionsort!

Even then, it's genuinely unclear to me what would be optimal here. Seems like linearizing the array to make it contiguous and avoid modulo logic could yield huge benefits.

@petrochenkov
Copy link
Contributor

sort is stable? o_O
At least a good thing is that it's not documented, it's still possible to fix it and add stable_sort as a separate function.

@Gankra Gankra added the A-docs label Oct 30, 2015
@Gankra
Copy link
Contributor

Gankra commented Oct 30, 2015

Changing that may be an unacceptable break.

@abonander
Copy link
Contributor

It'd be cool, if a little overkill, to have a crate with a bunch of different sorting functions. That might make a good crate for contain-rs.

@bluss
Copy link
Member

bluss commented Oct 31, 2015

Oh no, we haven't documented that it's a stable sort. That's a strange oversight.

@petrochenkov
Copy link
Contributor

@bluss

Oh no, we haven't documented that it's a stable sort.

"Oh yes" you mean?
Why should sort be stable by default? Stability is not usually needed. "Don't pay for what you don't use" and everything like this.

@Gankra
Copy link
Contributor

Gankra commented Oct 31, 2015

The current situation was definitely an active decision. I'm having trouble finding it, though.

I basically agree that it's a weird cost, but there's possibly an argument to be had that it's the "safer" default, and unstable_sort could easily be the opt-in to less reliable behavior.

@bluss
Copy link
Member

bluss commented Oct 31, 2015

@petrochenkov It should be stable by default because a stable sort is more versatile. Reordering equally ordered elements in sort is a potential gotcha, too, and Rust wants to be a nice language. The third party crate space is open to find other sorting algorithms with different tradeoffs.

@Gankra
Copy link
Contributor

Gankra commented Oct 31, 2015

Some discussion:

#11064

steveklabnik added a commit to steveklabnik/rust that referenced this issue Nov 4, 2015
steveklabnik added a commit to steveklabnik/rust that referenced this issue Nov 5, 2015
steveklabnik added a commit to steveklabnik/rust that referenced this issue Nov 5, 2015
@nico-abram
Copy link
Contributor

nico-abram commented Feb 22, 2020

Why was this closed? The PR linked by bors has nothing to do with VecDeque as far as I can see, and I cannot find a way to sort a VecDeque optimally in std. Someone (@hyeonu in the rust discord) mentioned From VecDeque for Vec and back guarantees to not allocate (In the docs), but it (1) requires ownership of the VecDeque (So can't use it to sort &mut VecDeque afaik) and (2) requires moving some data around before beginning the sort(Which shouldn't be necessary unless I'm missing something).

I'm not sure if there are other scenarios that it would make sense to sort that provide random access but are not contiguous memory from first to last (Which the current sort in std requires because it uses slices), but there might be.

@Mark-Simulacrum
Copy link
Member

It seems like this should be possible, at least. A slightly non-optimal implementation would be to sort the front and back slices separately and then swap elements back and forth as needed (second step of merge sort, basically).

@Alexendoo Alexendoo added A-collections Area: std::collections. C-feature-request Category: A feature request, i.e: not implemented / a PR. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. labels Mar 25, 2020
@retep998
Copy link
Member

retep998 commented Apr 1, 2020

Closed because of #69425 I assume

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-collections Area: std::collections. C-feature-request Category: A feature request, i.e: not implemented / a PR. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging a pull request may close this issue.