Add sort methods to Slice

[Maroo-b]

• Issue: suggestion: add sort method to Slice and StaticArray #6498

This PR depends on this fix: #7591

I extracted the sort logic from Array to a module. If this approach is approved, I'll try to add some docs for the module (or if someone has more skills with writing docs :) )

[asterite]

You can now rebase against master :-)

[straight-shoota]

What's the purpose of Sortable module? It doesn't define an actual interface, just some class methods can easily be placed in an existing namespace like Slice or Array. Only intro_sort is publicly visible at all (that might even be questionable).

[yxhuvud]

Is it possible to move sort, sort!, sort_by etc into Sortable?

[straight-shoota]

@yxhuvud Yes that should work. The exclamation methods need a tiny adjustment, but #to_unsafe returns the pointer in both Array and Slice.

But I'm not sure if this module would be much of a benefit. Duplicating the code for sort methods is one thing, but that's not really an issue. The question is, is having a Sortable interface useful to users?

[Maroo-b]

@straight-shoota
I extracted the sort logic from Array. Should I just rename the module to Sort for example and just require it in Array and Slice? or do you have another suggestion?

[asterite]

@Maroo-b Your changes (the Sort module) don't show up for some reason...

My thoughts:

• there's no need for a Sort or Sortable module that can be included. In fact there's no need for it to be generic, the T isn't used anywhere
• I agree that the sort helper methods (currently in Sortable) should exist somewhere where this logic can be shared, but let's not use global names like Sort or Sortable which can collide with user-defined types. I think just putting the helper methods in Slice or Pointer::SortUtils or Pointer::SortHelpers is good enough.

[Maroo-b]

@asterite thank you for the feedback!

Sure I agree, I'll move those methods to Pointer::SortUtils then :)

[straight-shoota]

IMHO the name Pointer::SortUtils doesn't make much sense. The sort algorithm is implemented on pointers, but conceptually, it operates on collections.

I'm still not convinced that we need a separate namespace for these methods anyway, but if we do, I'd call it Slice::Sort or Array::Sort. Or maybe Crystal::IntroSort?

[asterite]

The way I imagine it, the sort methods belong to Slice. Then Array simply does Slice.new(to_unsafe, size).sort.

[Maroo-b]

@straight-shoota @asterite just to confirm, should I move the sort logic to Slice and update Array to use Slice for sorting?
I'm concerned about creating a Slice instance just for sorting. is it fine?

[asterite]

It's fine, Slice is a struct. And yes, please move the methods to Slice. Thank you!

[straight-shoota]

Creating a struct is basically free of cost.

I'd suggest to add Array#to_slice as a shortcut for Slice.new(to_unsafe, size). Might be useful as a public method, too.

[bew]

@straight-shoota but we'd get back to #5173 (comment) ?

@RX14 wrote: We removed Array#to_slice because it would cause a segfault if you kept a reference to the slice returned from that method and the array was resized.

[straight-shoota]

Looks great!

I'd prefer to put the sort methods in a separate file (e.g. src/slice/sort.cr) though for some kind of compartmentalization.

[Maroo-b]

@straight-shoota thank you for the reviews, the PR was updated as suggested.

[straight-shoota]

Looks great

[bew]

What about moving the specs to spec/std/slice/sort_spec.cr ?

[Maroo-b]

@bew it's an option, but I prefer to keep Slice specs in one file for the moment.

[Maroo-b]

PR updated to resolve conflicts and apply changes from this PR: #7639

[Sija]

Seems like GTG 🚀

[sdogruyol]

Thank you @Maroo-b 🙏