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

Remove the Sort module. #1857

Merged
merged 1 commit into from Jul 13, 2018

Conversation

Projects
None yet
5 participants
@whitequark
Copy link
Member

commented Jun 23, 2018

See https://caml.inria.fr/mantis/view.php?id=7812.

It has been deprecated for 17 years (since 2001), and Sort.merge is documented to have undefined behavior when the lists being merged are not sorted in the first place.

@whitequark whitequark force-pushed the whitequark:remove-Sort branch 2 times, most recently from a36faa7 to 6e1af79 Jun 23, 2018

@xavierleroy
Copy link
Contributor

left a comment

It's not just that the Sort module has been documented as deprecated for a long time; also, the Sort functions have triggered a deprecation warning since 4.02. That's long enough to conclude that it's time to remove the Sort module.

@whitequark whitequark force-pushed the whitequark:remove-Sort branch from 6e1af79 to a575320 Jun 23, 2018

@nojb

This comment has been minimized.

Copy link
Contributor

commented Jun 24, 2018

The functional argument passed to List.sort in https://github.com/ocaml/ocaml/blob/trunk/testsuite/tests/tool-lexyacc/output.ml#L47 needs to be adjusted to return an int rather than a bool.

@whitequark whitequark force-pushed the whitequark:remove-Sort branch from a575320 to 931744b Jun 24, 2018

@whitequark

This comment has been minimized.

Copy link
Member Author

commented Jun 24, 2018

@nojb Thanks! Fixed (in the right direction, I think)

@@ -271,12 +271,12 @@ are two ways to fix this error:
type (without type variables) to \var{name}. For instance, instead of
writing
\begin{verbatim}
let sort_int_list = Sort.list (<)
let sort_int_list = List.sort (<)

This comment has been minimized.

Copy link
@damiendoligez

damiendoligez Jun 25, 2018

Member

This will give a type error. You should write: List.sort Stdlib.compare instead.

Same thing a few lines down.

This comment has been minimized.

Copy link
@whitequark

whitequark Jul 10, 2018

Author Member

Fixed.

Remove the Sort module. (PR7812)
It has been deprecated since 2000, shown a deprecation warning
since 4.02, and Sort.merge is documented to have undefined behavior
when the lists being merged are not sorted in the first place.

@whitequark whitequark force-pushed the whitequark:remove-Sort branch from 931744b to c12567f Jul 10, 2018

@alainfrisch

This comment has been minimized.

Copy link
Contributor

commented Jul 12, 2018

@damiendoligez Since your comment has been addressed, can you accept the PR?

@alainfrisch alainfrisch added the stdlib label Jul 12, 2018

@damiendoligez

This comment has been minimized.

Copy link
Member

commented Jul 12, 2018

I'm running an OPAM check on this PR (mostly out of curiosity). Let's wait a few more hours for the results.

@damiendoligez

This comment has been minimized.

Copy link
Member

commented Jul 13, 2018

FTR: this PR doesn't break any of the packages that currently compile with trunk.

@damiendoligez damiendoligez merged commit 1ebc9f5 into ocaml:trunk Jul 13, 2018

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@whitequark whitequark deleted the whitequark:remove-Sort branch Jul 14, 2018

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