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

Stabilize slice_sort_by_cached_key #58074

Merged
merged 2 commits into from
Feb 17, 2019
Merged

Stabilize slice_sort_by_cached_key #58074

merged 2 commits into from
Feb 17, 2019

Conversation

scottmcm
Copy link
Member

@scottmcm scottmcm commented Feb 2, 2019 •
edited
Loading

I was going to ask on the tracking issue (#34447), but decided to just send this and hope for an FCP here. The method was added last March by #48639.

Signature: https://doc.rust-lang.org/std/primitive.slice.html#method.sort_by_cached_key

impl [T] {
    pub fn sort_by_cached_key<K, F>(&mut self, f: F)
        where F: FnMut(&T) -> K, K: Ord;
}

That's an identical signature to the existing sort_by_key, so I think the questions are just naming, implementation, and the usual "do we want this?".

The implementation seems to have proven its use in rustc at least, which many uses: https://github.com/rust-lang/rust/search?l=Rust&q=sort_by_cached_key

(I'm asking because it's exactly what I just needed the other day:

    all_positions.sort_by_cached_key(|&n|
        data::CITIES.iter()
            .map(|x| *metric_closure.get_edge(n, x.pos).unwrap())
            .sum::<usize>()
    );

since caching that key is a pretty obviously good idea.)

Closes #34447

@scottmcm scottmcm added T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. needs-fcp This change is insta-stable, so needs a completed FCP to proceed. labels Feb 2, 2019
@rust-highfive

This comment has been minimized.

Sign in to view
@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Feb 2, 2019
@scottmcm
Copy link
Member Author

scottmcm commented Feb 2, 2019

Since this is needs-fcp, it needs someone from libs. Since they've p-FCP'd things recently, how about

r? @SimonSapin

@Centril Centril added the relnotes Marks issues that should be documented in the release notes of the next release. label Feb 2, 2019
@SimonSapin
Copy link
Contributor

@rfcbot fcp merge

@rfcbot
Copy link

rfcbot commented Feb 2, 2019 •
edited by dtolnay
Loading

Team member @SimonSapin has proposed to merge this. The next step is review by the rest of the tagged team members:

No concerns currently listed.

Once a majority of reviewers approve (and none object), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

@rfcbot rfcbot added proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. labels Feb 2, 2019
@SimonSapin SimonSapin added S-waiting-on-team Status: Awaiting decision from the relevant subteam (see the T-<team> label). and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Feb 2, 2019
@SimonSapin
Copy link
Contributor

r+ on the diff, good to merge once FCP is finished.

@rfcbot
Copy link

rfcbot commented Feb 5, 2019

🔔 This is now entering its final comment period, as per the review above. 🔔

@rfcbot rfcbot added final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. and removed proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. labels Feb 5, 2019
@bors

This comment has been minimized.

Sign in to view
@scottmcm scottmcm force-pushed the stabilize-sort_by_cached_key branch from 2827922 to 317f153 Compare February 13, 2019 06:27
@rfcbot
Copy link

rfcbot commented Feb 15, 2019

The final comment period, with a disposition to merge, as per the review above, is now complete.

@rfcbot rfcbot added finished-final-comment-period The final comment period is finished for this PR / Issue. and removed final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. labels Feb 15, 2019
@scottmcm
Copy link
Member Author

@bors r=SimonSapin

@bors
Copy link
Contributor

bors commented Feb 15, 2019

📌 Commit 317f153 has been approved by SimonSapin

@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-team Status: Awaiting decision from the relevant subteam (see the T-<team> label). labels Feb 15, 2019
@kennytm kennytm mentioned this pull request Feb 15, 2019
Merged
@bors
Copy link
Contributor

bors commented Feb 15, 2019

⌛ Testing commit 317f153 with merge 8bfe6ce9efdee35fadb833e90a5a019d68446c0e...

@bors
Copy link
Contributor

bors commented Feb 15, 2019

💔 Test failed - checks-travis

@rust-highfive
Copy link
Collaborator

Your PR failed on Travis (raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem.

Click to expand the log.

I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact @TimNN. (Feature Requests)

@bors bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Feb 15, 2019
@pietroalbini
Copy link
Member

@bors retry -- apparently we can't clone the repo anymore on macOS

@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 Feb 15, 2019
kennytm added a commit to kennytm/rust that referenced this pull request Feb 16, 2019
@kennytm
Rollup merge of rust-lang#58074 - scottmcm:stabilize-sort_by_cached_k…
f05e6bf 
…ey, r=SimonSapin

Stabilize slice_sort_by_cached_key

I was going to ask on the tracking issue (rust-lang#34447), but decided to just send this and hope for an FCP here.  The method was added last March by rust-lang#48639.

Signature: https://doc.rust-lang.org/std/primitive.slice.html#method.sort_by_cached_key
```rust
impl [T] {
    pub fn sort_by_cached_key<K, F>(&mut self, f: F)
        where F: FnMut(&T) -> K, K: Ord;
}
```

That's an identical signature to the existing `sort_by_key`, so I think the questions are just naming, implementation, and the usual "do we want this?".

The implementation seems to have proven its use in rustc at least, which many uses: https://github.com/rust-lang/rust/search?l=Rust&q=sort_by_cached_key

(I'm asking because it's exactly what I just needed the other day:
```rust
    all_positions.sort_by_cached_key(|&n|
        data::CITIES.iter()
            .map(|x| *metric_closure.get_edge(n, x.pos).unwrap())
            .sum::<usize>()
    );
```
since caching that key is a pretty obviously good idea.)

Closes rust-lang#34447
bors added a commit that referenced this pull request Feb 16, 2019
@bors
Auto merge of #58495 - kennytm:rollup, r=kennytm
f5f8284 
Rollup of 19 pull requests

Successful merges:

 - #57929 (Rustdoc remove old style files)
 - #57981 (Fix #57730)
 - #58074 (Stabilize slice_sort_by_cached_key)
 - #58196 (Add specific feature gate error for const-unstable features)
 - #58293 (Remove code for updating copyright years in generate-deriving-span-tests)
 - #58306 (Don't default on std crate when manipulating browser history)
 - #58359 (librustc_mir: use ? in impl_snapshot_for! macro)
 - #58395 (Instant::checked_duration_since)
 - #58429 (fix Box::into_unique effecitvely transmuting to a raw ptr)
 - #58433 (Update which libcore/liballoc tests Miri ignores, and document why)
 - #58438 (Use posix_spawn_file_actions_addchdir_np when possible)
 - #58440 (Whitelist the ARM v6 target-feature)
 - #58448 (rustdoc: mask `compiler_builtins` docs)
 - #58468 (split MaybeUninit into several features, expand docs a bit)
 - #58477 (Fix the syntax error in publish_toolstate.py)
 - #58479 (compile-pass test for #53606)
 - #58489 (Fix runtime error in generate-keyword-tests)
 - #58496 (Fix documentation for std::path::PathBuf::pop)
 - #58509 (Notify myself when Clippy toolstate changes)
bors added a commit that referenced this pull request Feb 17, 2019
@bors
Auto merge of #58495 - kennytm:rollup, r=kennytm
8af675a 
Rollup of 19 pull requests

Successful merges:

 - #57929 (Rustdoc remove old style files)
 - #57981 (Fix #57730)
 - #58074 (Stabilize slice_sort_by_cached_key)
 - #58196 (Add specific feature gate error for const-unstable features)
 - #58293 (Remove code for updating copyright years in generate-deriving-span-tests)
 - #58306 (Don't default on std crate when manipulating browser history)
 - #58359 (librustc_mir: use ? in impl_snapshot_for! macro)
 - #58395 (Instant::checked_duration_since)
 - #58429 (fix Box::into_unique effecitvely transmuting to a raw ptr)
 - #58433 (Update which libcore/liballoc tests Miri ignores, and document why)
 - #58438 (Use posix_spawn_file_actions_addchdir_np when possible)
 - #58440 (Whitelist the ARM v6 target-feature)
 - #58448 (rustdoc: mask `compiler_builtins` docs)
 - #58468 (split MaybeUninit into several features, expand docs a bit)
 - #58479 (compile-pass test for #53606)
 - #58489 (Fix runtime error in generate-keyword-tests)
 - #58496 (Fix documentation for std::path::PathBuf::pop)
 - #58509 (Notify myself when Clippy toolstate changes)
 - #58521 (Fix tracking issue for error iterators)
@bors bors merged commit 317f153 into rust-lang:master Feb 17, 2019
@scottmcm scottmcm deleted the stabilize-sort_by_cached_key branch February 18, 2019 00:17
@Centril Centril added this to the 1.34 milestone Apr 27, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@SimonSapin SimonSapin

Labels
disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. finished-final-comment-period The final comment period is finished for this PR / Issue. needs-fcp This change is insta-stable, so needs a completed FCP to proceed. relnotes Marks issues that should be documented in the release notes of the next release. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Milestone
1.34
Development

Successfully merging this pull request may close these issues.

Tracking issue for sorting by expensive-to-compute keys (feature slice_sort_by_cached_key)
8 participants
@scottmcm @rust-highfive @SimonSapin @rfcbot @bors @pietroalbini @Centril @Mark-Simulacrum