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.

Sign up for GitHub

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

Jump to bottom

Add [f32]::sort_floats and [f64]::sort_floats #93397

Merged
merged 1 commit into from
Jul 23, 2022
Merged

Add [f32]::sort_floats and [f64]::sort_floats #93397

merged 1 commit into from
Jul 23, 2022

Conversation

joshtriplett
Copy link
Member

@joshtriplett joshtriplett commented Jan 27, 2022 •
edited
Loading

It's inconvenient to sort a slice or Vec of floats, compared to sorting integers. To simplify numeric code, add a convenience method to [f32] and [f64] to sort them using sort_unstable_by with total_cmp.

@rust-highfive
Copy link
Collaborator

r? @kennytm

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jan 27, 2022
@joshtriplett joshtriplett mentioned this pull request Jan 27, 2022
Open
3 tasks
@joshtriplett
Copy link
Member Author

Discussed on Zulip at https://rust-lang.zulipchat.com/#narrow/stream/219381-t-libs/topic/float.20total_cmp .

@rust-log-analyzer

This comment has been minimized.

Sign in to view
@cuviper
Copy link
Member

cuviper commented Jan 28, 2022

They'll need distinct new lang items, just like slice_u8 for its ascii methods.

@rustbot rustbot added T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. labels Jan 28, 2022
@joshtriplett
Copy link
Member Author

@cuviper Thanks, done.

@joshtriplett joshtriplett added the T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. label Jan 28, 2022
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@joshtriplett
Copy link
Member Author

Marked as cfg(not(bootstrap)) since the bootstrap compiler doesn't have these lang items.

@rust-log-analyzer

This comment has been minimized.

Sign in to view
@joshtriplett joshtriplett force-pushed the sort-floats branch 3 times, most recently from 1f05d40 to b16f09b Compare January 28, 2022 22:21
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@joshtriplett
Copy link
Member Author

...works better if I actually sort the sorted array in the right order...

@rust-log-analyzer

This comment has been minimized.

Sign in to view
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@joshtriplett
Copy link
Member Author

This looks like the mir-opt segfault hitting CI for various PRs right now (#93447); waiting for that to get sorted out before retrying.

@joshtriplett joshtriplett changed the title Add [f32]::sort_floats and [f64]::sort_floats Add [f32]::sort and [f64]::sort Jan 30, 2022
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@joshtriplett
Add [f32]::sort_floats and [f64]::sort_floats
bded8fc 
It's inconvenient to sort a slice or Vec of floats, compared to sorting
integers. To simplify numeric code, add a convenience method to `[f32]`
and `[f64]` to sort them using `sort_unstable_by` with `total_cmp`.
@joshtriplett
Copy link
Member Author

I've rebased this to eliminate the merge conflicts. Thanks to #94963 this got a lot simpler.

@joshtriplett
Copy link
Member Author

I continue to believe that this doesn't hide NaN behavior any more than total_cmp itself does.

However, if people feel that a name like sort_floats_total would address this concern more explicitly, I can rename the method. Would that address the objections, or are there other concerns beyond that?

@Dylan-DPC Dylan-DPC added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jul 16, 2022
@pnkfelix
Copy link
Member

r? rust-lang/compiler

@apiraino
Copy link
Contributor

@rustbot r? rust/compiler

@rust-highfive rust-highfive assigned lcnr and petrochenkov and unassigned kennytm and lcnr Jul 21, 2022
@petrochenkov
Copy link
Contributor

petrochenkov commented Jul 21, 2022 •
edited
Loading

Not compiler, @rustbot r? rust/libs-api

@petrochenkov
Copy link
Contributor

petrochenkov commented Jul 21, 2022 •
edited
Loading

@rustbot r? rust-lang/libs-api

@petrochenkov
Copy link
Contributor

petrochenkov commented Jul 21, 2022 •
edited
Loading

@rustbot r? rust-lang/library

@petrochenkov
Copy link
Contributor

r? @rust-lang/libs-api

@petrochenkov
Copy link
Contributor

(╯°□°)╯︵ ┻━┻
r? @Amanieu

@Amanieu
Copy link
Member

Amanieu commented Jul 22, 2022

I find these questionably useful, but I guess we can defer this discussion until stabilization.

@bors r+

@bors
Copy link
Contributor

bors commented Jul 22, 2022

📌 Commit bded8fc has been approved by Amanieu

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jul 22, 2022
@bors
Copy link
Contributor

bors commented Jul 23, 2022

⌛ Testing commit bded8fc with merge ed793d8...

@bors
Copy link
Contributor

bors commented Jul 23, 2022

☀️ Test successful - checks-actions
Approved by: Amanieu
Pushing ed793d8 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Jul 23, 2022
@bors bors merged commit ed793d8 into rust-lang:master Jul 23, 2022
@rustbot rustbot added this to the 1.64.0 milestone Jul 23, 2022
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (ed793d8): comparison url.

Instruction count

This benchmark run did not return any relevant results for this metric.

Max RSS (memory usage)

Results
  • Primary benchmarks: no relevant changes found
  • Secondary benchmarks: mixed results
mean1 max count2
Regressions 😿
(primary)		 N/A N/A 0
Regressions 😿
(secondary)		 2.3% 2.3% 1
Improvements 🎉
(primary)		 N/A N/A 0
Improvements 🎉
(secondary)		 -3.0% -3.0% 1
All 😿🎉 (primary) N/A N/A 0

Cycles

Results
  • Primary benchmarks: 😿 relevant regression found
  • Secondary benchmarks: no relevant changes found
mean1 max count2
Regressions 😿
(primary)		 3.4% 3.4% 1
Regressions 😿
(secondary)		 N/A N/A 0
Improvements 🎉
(primary)		 N/A N/A 0
Improvements 🎉
(secondary)		 N/A N/A 0
All 😿🎉 (primary) 3.4% 3.4% 1

If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf.

@rustbot label: -perf-regression

Footnotes

  1. the arithmetic mean of the percent change ↩ ↩2

  2. number of relevant changes ↩ ↩2

@joshtriplett joshtriplett deleted the sort-floats branch July 28, 2022 04:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@WaffleLapkin WaffleLapkin WaffleLapkin left review comments

@leonardo-m leonardo-m leonardo-m left review comments

@scottmcm scottmcm scottmcm left review comments

Assignees

@Amanieu Amanieu

Labels
merged-by-bors This PR was explicitly merged by bors. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue.
Projects
None yet
Milestone
1.64.0
Development

Successfully merging this pull request may close these issues.