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

Add sort methods to Indexable::Mutable #11195

Conversation

straight-shoota
Copy link
Member

@straight-shoota straight-shoota commented Sep 9, 2021

This patch moves the sort methods from Slice and Array into Indexable::Mutable as suggested in #11057 (comment). It is an alternative to #11057.

All mutable indexables support mutating sort, although the default implementation is not very efficient because it duplicates the collection to a slice for sorting. Array (and Slice obviously) overrides the mutating sort methods to provide a more efficient implementation by directly operating on the buffer. A similar approach can be used for other data types that represent the values in such a way (e.g. StaticArray: #10889).

Non-mutating sort methods are certainly not limited to mutable collections, but mutable collections offer a simple implementation, based on #dup and the mutating sort methods. #dup is not required to be implemented on inheriting types, but it should be pretty common (all Indexable::Mutable types in stdlib implement #dup except StaticArray). So this is a kind of weak dependency, but IMO it's acceptable.
We could consider adding #dup as an abstract def to Indexable::Mutable to make it explicit. But I'm not sure that's really necessary.

Closes #11057

@HertzDevil
Copy link
Contributor

Returning covariant containers from #sort is something that has never been done before in Enumerable or Indexable. It would imply that other methods could be extended to return covariantly this way:

module Indexable::Mutable(T)
  def reverse
    x = dup
    x.reverse!
  end

  def rotate
    x = dup
    x.rotate!
  end

  def shuffle(random)
    x = dup
    x.shuffle!(random)
  end

  def map(& : T -> T) # same return type only
    x = dup
    x.map! { |elem| yield elem }
  end
end

I think this warrants more discussion. For now only the actual mutating methods should be added.

@straight-shoota
Copy link
Member Author

What does "covariant" mean in this context? That we can't assume which type the implementation of #dup returns?

But I agreed that these methods should probably not bei merged like that.

@yxhuvud
Copy link
Contributor

yxhuvud commented Sep 12, 2021

@HertzDevil Those examples may work, but those are pretty inefficient as implementations go as it would force all contents to be streamed through memory twice. Which may be noticable for big containers. That may be ok for O(nlog) operations like sort, but should probably be avoided for operations that are O(n) in any case.

@beta-ziliani
Copy link
Member

@HertzDevil care to expand a bit? I don't think I get your comment.

Copy link
Member

@beta-ziliani beta-ziliani left a comment

Choose a reason for hiding this comment

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

LGTM. In the future I think it's best to add the dup method somewhere in the hierarchy, although I'm not sure where it should be.

@beta-ziliani beta-ziliani added this to the 1.2.0 milestone Sep 28, 2021
@beta-ziliani beta-ziliani removed this from the 1.2.0 milestone Sep 28, 2021
@straight-shoota
Copy link
Member Author

Closing. The main parts were merged in #11254

@straight-shoota straight-shoota deleted the feature/indexable_mutable-sort branch November 18, 2021 19:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants