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

Improve readability in a few sorts #52692

Merged
merged 1 commit into from
Jul 26, 2018
Merged

Conversation

ljedrz
Copy link
Contributor

@ljedrz ljedrz commented Jul 25, 2018

Use sort_by_key where possible.

@rust-highfive
Copy link
Collaborator

r? @petrochenkov

(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 Jul 25, 2018
@petrochenkov
Copy link
Contributor

@bors r+ rollup

@bors
Copy link
Contributor

bors commented Jul 25, 2018

📌 Commit f653bf4 has been approved by petrochenkov

@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 25, 2018
@leonardo-m
Copy link

That improves readability, but keep sort_by_key allocates some new memory, and this sometimes makes the code slower than using a cmp without allocations. So in theory it's better to benchmark before/after this change.

@scottmcm
Copy link
Member

@leonardo-m Are you sure about that? The recent sort_by_cached_key pre-computes the keys into new memory, but ordinary sort_by_key seems to just be the same as the cmp closure:

merge_sort(self, |a, b| f(a).lt(&f(b)));

@leonardo-m
Copy link

OK, right

Mark-Simulacrum added a commit to Mark-Simulacrum/rust that referenced this pull request Jul 26, 2018
…enkov

Improve readability in a few sorts

Use `sort_by_key` where possible.
bors added a commit that referenced this pull request Jul 26, 2018
Rollup of 16 pull requests

Successful merges:

 - #52558 (Add tests for ICEs which no longer repro)
 - #52610 (Clarify what a task is)
 - #52617 (Don't match on region kinds when reporting NLL errors)
 - #52635 (Fix #[linkage] propagation though generic functions)
 - #52647 (Suggest to take and ignore args while closure args count mismatching)
 - #52649 (Point spans to inner elements of format strings)
 - #52654 (Format linker args in a way that works for gcc and ld)
 - #52667 (update the stdsimd submodule)
 - #52674 (Impl Executor for Box<E: Executor>)
 - #52690 (ARM: expose `rclass` and `dsp` target features)
 - #52692 (Improve readability in a few sorts)
 - #52695 (Hide some lints which are not quite right the way they are reported to the user)
 - #52718 (State default capacity for BufReader/BufWriter)
 - #52721 (std::ops::Try impl for std::task::Poll)
 - #52723 (rustc: Register crates under their real names)
 - #52734 (sparc ABI issue - structure returning from function is returned in 64bit registers (with tests))

Failed merges:

 - #52678 ([NLL] Use better spans in some errors)

r? @ghost
@bors bors merged commit f653bf4 into rust-lang:master Jul 26, 2018
@ljedrz ljedrz deleted the sort_improvements branch July 26, 2018 19:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants