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

Remove calls to mem::forget and mem::replace in Option::get_or_insert_with. #111301

Merged
merged 1 commit into from
May 7, 2023
Merged

Remove calls to mem::forget and mem::replace in Option::get_or_insert_with. #111301

merged 1 commit into from
May 7, 2023

Conversation

JohnBobbo96
Copy link
Contributor

@JohnBobbo96 JohnBobbo96 commented May 6, 2023 •
edited
Loading

This removes the unneeded calls to mem::forget and mem::replace in Option::get_or_insert_with.

@rustbot
Copy link
Collaborator

rustbot commented May 6, 2023

r? @scottmcm

(rustbot has picked a reviewer for you, use r? to override)

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-libs Relevant to the library team, which will review and decide on the PR/issue. labels May 6, 2023
@rustbot
Copy link
Collaborator

rustbot commented May 6, 2023

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

scottmcm
scottmcm reviewed May 6, 2023
View reviewed changes
library/core/src/option.rs Outdated
@@ -1554,8 +1554,11 @@ impl<T> Option<T> {
pub fn insert(&mut self, value: T) -> &mut T {
*self = Some(value);

// SAFETY: the code above just filled the option
unsafe { self.as_mut().unwrap_unchecked() }
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Was there some problem you were hitting because of the function calls? Is there a test you could add to detect whatever it was, to keep it from happening again in future?

Especially with MIR inlining on for core now, it's not clear to me that open-coding these is better than leaving them alone.

@rustbot author

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wasn't having any specific problems, i just thought it might be better to merge the two functions into a single match statement, but if you think it would be better to just leave them, i can get rid of those changes.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Without any specific reason, I'd like to just leave it as-is. Otherwise it's equally valid to "might be better to use the clearer combinators instead of writing them out by hand", and could flip-flop back and forth in the implementation.

If you cut this down to just the forget-replace cleanup, then I'd approve it.

@rustbot author

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fair enough, i have reverted the other changes.

@rustbot rustbot 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 May 6, 2023
scottmcm
scottmcm reviewed May 6, 2023
View reviewed changes
library/core/src/option.rs
// here and wants us to ensure `T` can be dropped at compile time.
mem::forget(mem::replace(self, Some(f())))
if let None = self {
*self = Some(f());
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change makes sense to me, though -- the forget+replace is a residue from when this was unstably const, before that was undone.

@JohnBobbo96
Copy link
Contributor Author

@rustbot ready

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. 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. S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels May 6, 2023
@JohnBobbo96
Remove unneeded calls to mem::forget
ec7fcdc 
and `mem::replace` in `Option::get_or_insert_with`.
@JohnBobbo96 JohnBobbo96 changed the title Replace some extra function calls in core::option with match statements. Remove calls to mem::forget and mem::replace in Option::get_or_insert_with. May 6, 2023
@JohnBobbo96
Copy link
Contributor Author

@rustbot ready

@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 May 6, 2023
@scottmcm
Copy link
Member

scottmcm commented May 6, 2023

Thanks!

@bors r+ rollup

@bors
Copy link
Contributor

bors commented May 6, 2023

📌 Commit ec7fcdc has been approved by scottmcm

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 May 6, 2023
@JohnTitor JohnTitor mentioned this pull request May 7, 2023
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request May 7, 2023
@bors
Auto merge of rust-lang#111311 - JohnTitor:rollup-vfpjm0d, r=JohnTitor
8cc75b5 
Rollup of 7 pull requests

Successful merges:

 - rust-lang#105583 (Operand::extract_field: only cast llval if it's a pointer and replace bitcast w/ pointercast.)
 - rust-lang#110094 (clean up `transmute`s in `core`)
 - rust-lang#111150 (added TraitAlias to check_item() for missing_docs)
 - rust-lang#111293 (rustc --explain E0726 - grammar fixing (it's => its + add a `the` where it felt right to do so))
 - rust-lang#111300 (Emit while_true lint spanning the entire loop condition)
 - rust-lang#111301 (Remove calls to `mem::forget` and `mem::replace` in `Option::get_or_insert_with`.)
 - rust-lang#111303 (update Rust Unstable Book docs for `--extern force`)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 816e029 into rust-lang:master May 7, 2023
@rustbot rustbot added this to the 1.71.0 milestone May 7, 2023
@JohnBobbo96 JohnBobbo96 deleted the cleanup_option_insert_methods branch May 7, 2023 21:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@scottmcm scottmcm scottmcm left review comments

Assignees

@scottmcm scottmcm

Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-libs Relevant to the library team, which will review and decide on the PR/issue.
Projects
None yet
Milestone
1.71.0
Development

Successfully merging this pull request may close these issues.
4 participants
@JohnBobbo96 @rustbot @scottmcm @bors