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 Slice #7597

Merged
merged 1 commit into from May 30, 2019

Conversation

@Maroo-b
Copy link
Contributor

commented Mar 26, 2019

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

This comment has been minimized.

Copy link
Member

commented Mar 26, 2019

You can now rebase against master :-)

@Maroo-b Maroo-b force-pushed the Maroo-b:6498_add_sorting_to_slice branch from 98e769f to eb98a1d Mar 27, 2019

@straight-shoota

This comment has been minimized.

Copy link
Member

commented Mar 27, 2019

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

This comment has been minimized.

Copy link
Contributor

commented Mar 27, 2019

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

@straight-shoota

This comment has been minimized.

Copy link
Member

commented Mar 27, 2019

@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

This comment has been minimized.

Copy link
Contributor Author

commented Mar 27, 2019

@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

This comment has been minimized.

Copy link
Member

commented Mar 27, 2019

@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

This comment has been minimized.

Copy link
Contributor Author

commented Mar 27, 2019

@asterite thank you for the feedback!

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

@Maroo-b Maroo-b force-pushed the Maroo-b:6498_add_sorting_to_slice branch 3 times, most recently from 081681b to 151ffea Mar 27, 2019

@straight-shoota

This comment has been minimized.

Copy link
Member

commented Mar 27, 2019

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

This comment has been minimized.

Copy link
Member

commented Mar 27, 2019

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

src/slice.cr Outdated Show resolved Hide resolved
src/pointer/sort_utils.cr Outdated Show resolved Hide resolved
src/pointer/sort_utils.cr Outdated Show resolved Hide resolved
src/pointer/sort_utils.cr Outdated Show resolved Hide resolved
src/slice.cr Outdated Show resolved Hide resolved
src/slice.cr Show resolved Hide resolved
src/slice.cr Outdated Show resolved Hide resolved
src/slice.cr Show resolved Hide resolved
src/slice.cr Outdated Show resolved Hide resolved
src/pointer/sort_utils.cr Outdated Show resolved Hide resolved
@Maroo-b

This comment has been minimized.

Copy link
Contributor Author

commented Mar 28, 2019

@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

This comment has been minimized.

Copy link
Member

commented Mar 28, 2019

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

@straight-shoota

This comment has been minimized.

Copy link
Member

commented Mar 28, 2019

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

This comment has been minimized.

Copy link
Contributor

commented Mar 28, 2019

@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.

@Maroo-b Maroo-b force-pushed the Maroo-b:6498_add_sorting_to_slice branch from 151ffea to 2d8fb36 Mar 28, 2019

@straight-shoota

This comment has been minimized.

Copy link
Member

commented Mar 28, 2019

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 Maroo-b force-pushed the Maroo-b:6498_add_sorting_to_slice branch 2 times, most recently from 722218f to d6d8a5e Mar 29, 2019

@Maroo-b

This comment has been minimized.

Copy link
Contributor Author

commented Mar 29, 2019

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

@straight-shoota
Copy link
Member

left a comment

Looks great

src/slice/sort.cr Outdated Show resolved Hide resolved
@bew

This comment has been minimized.

Copy link
Contributor

commented Mar 29, 2019

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

@Maroo-b

This comment has been minimized.

Copy link
Contributor Author

commented Mar 29, 2019

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

@Maroo-b Maroo-b force-pushed the Maroo-b:6498_add_sorting_to_slice branch from d6d8a5e to 2d58ecc Mar 29, 2019

@Maroo-b Maroo-b force-pushed the Maroo-b:6498_add_sorting_to_slice branch from 2d58ecc to a7ef60a Apr 28, 2019

@Maroo-b Maroo-b force-pushed the Maroo-b:6498_add_sorting_to_slice branch from a7ef60a to 93ebc49 Apr 28, 2019

@Maroo-b

This comment has been minimized.

Copy link
Contributor Author

commented Apr 28, 2019

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

@Sija

This comment has been minimized.

Copy link
Contributor

commented May 29, 2019

Seems like GTG 🚀

@sdogruyol
Copy link
Member

left a comment

Thank you @Maroo-b 🙏

@asterite asterite added this to the 0.29.0 milestone May 30, 2019

@asterite asterite merged commit f466695 into crystal-lang:master May 30, 2019

5 checks passed

ci/circleci: check_format Your tests passed on CircleCI!
Details
ci/circleci: test_darwin Your tests passed on CircleCI!
Details
ci/circleci: test_linux Your tests passed on CircleCI!
Details
ci/circleci: test_linux32 Your tests passed on CircleCI!
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@Maroo-b Maroo-b deleted the Maroo-b:6498_add_sorting_to_slice branch Jun 3, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
8 participants
You can’t perform that action at this time.