Skip to content

Implement Option::take_if - #98935

Merged
bors merged 1 commit into
rust-lang:masterfrom
kellerkindt:option_retain
Aug 7, 2023
Merged

Implement Option::take_if#98935
bors merged 1 commit into
rust-lang:masterfrom
kellerkindt:option_retain

Conversation

@kellerkindt

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

Copy link
Copy Markdown
Contributor

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

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

rustbot commented Jul 5, 2022

Copy link
Copy Markdown
Collaborator

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

rust-highfive commented Jul 5, 2022

Copy link
Copy Markdown
Contributor

r? @Mark-Simulacrum

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

@rust-highfiverust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jul 5, 2022
@kellerkindtkellerkindt mentioned this pull request Jul 5, 2022
Closed
5 tasks
@kellerkindt
kellerkindtforce-pushed the option_retain branch 3 times, most recently from f00d2ef to 49ae5d7CompareJuly 6, 2022 07:41
@kellerkindtkellerkindt changed the title Implement Option::retain and Option::retain_mutImplement Option::retainJul 6, 2022
@kellerkindt

kellerkindt commented Jul 7, 2022

Copy link
Copy Markdown
ContributorAuthor

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

Mark-Simulacrum commented Jul 14, 2022

Copy link
Copy Markdown
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-SimulacrumMark-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

kellerkindt commented Jul 14, 2022

Copy link
Copy Markdown
ContributorAuthor

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

@rustbotrustbot added T-libs-api [DEPRECATED; DO NOT USE] and removed T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Jul 14, 2022
@kellerkindt

kellerkindt commented Jul 15, 2022•
edited
Loading

Copy link
Copy Markdown
ContributorAuthor

@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

Mark-Simulacrum commented Jul 15, 2022

Copy link
Copy Markdown
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.

@kellerkindtkellerkindt mentioned this pull request Jul 16, 2022
Closed
@kellerkindt

kellerkindt commented Jul 16, 2022

Copy link
Copy Markdown
ContributorAuthor

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

@JohnCSimonJohnCSimon 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

JohnCSimon commented Oct 2, 2022

Copy link
Copy Markdown

still waiting on ACP.

@JohnCSimonJohnCSimon 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

JohnCSimon commented Nov 6, 2022

Copy link
Copy Markdown

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

@kellerkindt

kellerkindt commented Nov 6, 2022•
edited
Loading

Copy link
Copy Markdown
ContributorAuthor

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

@JohnCSimon

JohnCSimon commented Nov 27, 2022

Copy link
Copy Markdown

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-DPCDylan-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-DPCDylan-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
Comment threadlibrary/core/src/option.rs Outdated
Comment threadlibrary/core/src/option.rs Outdated
@Mark-SimulacrumMark-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
@kellerkindtkellerkindt changed the title Implement Option::retainImplement Option::take_ifJul 31, 2023
@kellerkindt
kellerkindtforce-pushed the option_retain branch 3 times, most recently from 5ab59b9 to bcf5b9cCompareJuly 31, 2023 13:46
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@kellerkindt

kellerkindt commented Jul 31, 2023

Copy link
Copy Markdown
ContributorAuthor

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

@rustbotrustbot 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

Mark-Simulacrum commented Aug 6, 2023

Copy link
Copy Markdown
Member

@bors r+ rollup

@bors

bors commented Aug 6, 2023

Copy link
Copy Markdown
Collaborator

📌 Commit 5419abd has been approved by Mark-Simulacrum

It is now in the queue for this repository.

@borsbors 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
@matthiaskrgrmatthiaskrgr 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 ofrust-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:masterAug 7, 2023
@rustbotrustbot added this to the 1.73.0 milestone Aug 7, 2023
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@Mark-SimulacrumMark-SimulacrumMark-Simulacrum left review comments

Assignees

@Mark-SimulacrumMark-Simulacrum

Labels

S-waiting-on-borsStatus: Waiting on bors to run and complete tests. Bors will change the label on completion.T-libs-api[DEPRECATED; DO NOT USE]

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