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

Implement Option::take_if #98935

Merged
merged 1 commit into from
Aug 7, 2023
Merged

Implement Option::take_if #98935

merged 1 commit into from
Aug 7, 2023

Conversation

kellerkindt
Copy link
Contributor

@kellerkindt kellerkindt commented Jul 5, 2022 •
edited by Dylan-DPC
Loading

Tracking issue: #98934
ACP: rust-lang/libs-team#70 [accepted]

@rustbot rustbot added the T-libs Relevant to the library team, which will review and decide on the PR/issue. label Jul 5, 2022
@rustbot
Copy link
Collaborator

rustbot commented Jul 5, 2022

Hey! It looks like you've submitted a new PR for the library teams!

If this PR contains changes to any rust-lang/rust public library APIs then please comment with @rustbot label +T-libs-api -T-libs to tag it appropriately. If this PR contains changes to any unstable APIs please edit the PR description to add a link to the relevant API Change Proposal or create one if you haven't already. If you're unsure where your change falls no worries, just leave it as is and the reviewer will take a look and make a decision to forward on if necessary.

Examples of T-libs-api changes:

  • Stabilizing library features
  • Introducing insta-stable changes such as new implementations of existing stable traits on existing stable types
  • Introducing new or changing existing unstable library APIs (excluding permanently unstable features / features without a tracking issue)
  • Changing public documentation in ways that create new stability guarantees
  • Changing observable runtime behavior of library APIs

@rust-highfive
Copy link
Collaborator

r? @Mark-Simulacrum

(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 5, 2022
@kellerkindt kellerkindt mentioned this pull request Jul 5, 2022
Closed
5 tasks
@kellerkindt kellerkindt force-pushed the option_retain branch 3 times, most recently from f00d2ef to 49ae5d7 Compare July 6, 2022 07:41
@kellerkindt kellerkindt changed the title Implement Option::retain and Option::retain_mut Implement Option::retain Jul 6, 2022
@kellerkindt
Copy link
Contributor Author

Hey @Mark-Simulacrum, further input needed for the trait bounds. I copied them over from the Option::filter impl, but I am not familiar with the whole ~const thing. Could you thake a look? :)

@Mark-Simulacrum
Copy link
Member

Hi! Per the rustbot comment (#98935 (comment)), new APIs should be proposed as a T-libs-api API change proposal -- has one been filed for this? Can you link to it?

With regards to the const bounds, my recommendation is to drop the const_unstable + const part of this function for now; the story around ~const is likely to shift significantly and isn't near stabilization; we don't need to worry about it for new APIs being added. (It seems to compile, based on CI, so in that case it's likely fine to leave them as-is; not clear if you're indicating there's some problem...).

@Mark-Simulacrum Mark-Simulacrum added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jul 14, 2022
@kellerkindt
Copy link
Contributor Author

@rustbot label +T-libs-api -T-libs

@rustbot rustbot added T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. and removed T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Jul 14, 2022
@kellerkindt
Copy link
Contributor Author

kellerkindt commented Jul 15, 2022 •
edited
Loading

@Mark-Simulacrum Sorry, I only have a tracking issue linked. But there is an example why I think this change is useful. I also kinda hoped this change is small enough for If the new feature is small, it may be fine to skip the RFC process ... which is not the same as the API Change Proposal as it seems ... my bad 😅

I don't know if there are issues with ~const (as you said, CI seems to be happy with it... so...). I wanted to point it out so you don't assume I plead for it being correct.

EDIT: I added Problem statement and Motivation, use-cases to the tracking issue

@Mark-Simulacrum
Copy link
Member

The tracking issue looks pretty good! However, typically our process is that we:

  • File an ACP issue, pretty much with the text you've put in the tracking issue
  • In parallel with that (or after), file a PR linking to the proposal issue and file a tracking issue for the feature
  • Then, the PR gets reviewed and merged

You definitely don't need to write a full RFC for this -- just needs that issue on the libs-team repo so that they can review and approve the change going in.

@kellerkindt kellerkindt mentioned this pull request Jul 16, 2022
Closed
@kellerkindt
Copy link
Contributor Author

You definitely don't need to write a full RFC for this -- just needs that issue on the libs-team repo so that they can review and approve the change going in.

Thanks for clarification :)
ACP: rust-lang/libs-team#70

@JohnCSimon JohnCSimon added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. S-waiting-on-ACP Status: PR has an ACP and is waiting for the ACP to complete. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Aug 13, 2022
@JohnCSimon
Copy link
Member

still waiting on ACP.

@JohnCSimon JohnCSimon added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Nov 6, 2022
@JohnCSimon
Copy link
Member

@kellerkindt I guess this this is still waiting on ACP?

@kellerkindt
Copy link
Contributor Author

kellerkindt commented Nov 6, 2022 •
edited
Loading

Yeah, there seems to be no movement? Or did I miss something to start the actual discussion?

@JohnCSimon
Copy link
Member

Yeah, there seems to be no movement? Or did I miss something to start the actual discussion?

I don't know, sorry. Maybe ask people on zulip - https://rust-lang.zulipchat.com/
like @the8472 who commented here rust-lang/libs-team#70 (comment)

@Dylan-DPC Dylan-DPC removed the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label May 14, 2023
@Dylan-DPC Dylan-DPC added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-ACP Status: PR has an ACP and is waiting for the ACP to complete. S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jul 21, 2023
Mark-Simulacrum
Mark-Simulacrum reviewed Jul 31, 2023
View reviewed changes
library/core/src/option.rs Outdated Show resolved Hide resolved
library/core/src/option.rs Outdated Show resolved Hide resolved
@Mark-Simulacrum Mark-Simulacrum added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jul 31, 2023
@kellerkindt kellerkindt changed the title Implement Option::retain Implement Option::take_if Jul 31, 2023
@kellerkindt kellerkindt force-pushed the option_retain branch 3 times, most recently from 5ab59b9 to bcf5b9c Compare July 31, 2023 13:46
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@kellerkindt
Copy link
Contributor Author

@rustbot modify labels: -S-waiting-on-author +S-waiting-on-review

@rustbot rustbot 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 31, 2023
@Mark-Simulacrum
Copy link
Member

@bors r+ rollup

@bors
Copy link
Contributor

bors commented Aug 6, 2023

📌 Commit 5419abd has been approved by Mark-Simulacrum

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 Aug 6, 2023
@matthiaskrgr matthiaskrgr mentioned this pull request Aug 7, 2023
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request Aug 7, 2023
@bors
Auto merge of rust-lang#114569 - matthiaskrgr:rollup-p8tcxtz, r=matth…
4a71a05 
…iaskrgr

Rollup of 8 pull requests

Successful merges:

 - rust-lang#98935 (Implement `Option::take_if`)
 - rust-lang#114093 (Add regression test for `echo 'mod unknown;' | rustc -`)
 - rust-lang#114229 (Nest tests/codegen/sanitizer*.rs tests in sanitizer dir)
 - rust-lang#114230 (Nest other codegen test topics)
 - rust-lang#114362 (string.rs: remove "Basic usage" text)
 - rust-lang#114365 (str.rs: remove "Basic usage" text)
 - rust-lang#114382 (Add a new `compare_bytes` intrinsic instead of calling `memcmp` directly)
 - rust-lang#114549 (Style fix and refactor on resolve diagnostics)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit bab20b4 into rust-lang:master Aug 7, 2023
@rustbot rustbot added this to the 1.73.0 milestone Aug 7, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Mark-Simulacrum Mark-Simulacrum Mark-Simulacrum left review comments

Assignees

@Mark-Simulacrum Mark-Simulacrum

Labels
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.73.0
Development

Successfully merging this pull request may close these issues.
8 participants
@kellerkindt @rustbot @rust-highfive @Mark-Simulacrum @JohnCSimon @rust-log-analyzer @bors @Dylan-DPC