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

Discourage overuse of mem::forget #53503

Merged
merged 1 commit into from
Aug 24, 2018
Merged

Discourage overuse of mem::forget #53503

merged 1 commit into from
Aug 24, 2018

Conversation

kornelski
Copy link
Contributor

Some uses of mem::forget have been replaced by better methods of Box, so I've removed obsoleted use-cases from these docs.

I've removed emphasis on leaking, because it's not obvious mem::forget does not guarantee leaking of memory: memory of stack-allocated objects and values partially moved out of Box will still be freed. That's a potential error when used to pass objects to FFI, so it's better to direct users to Box::into_raw instead.

@rust-highfive
Copy link
Collaborator

r? @dtolnay

(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 Aug 19, 2018
@dtolnay
Copy link
Member

dtolnay commented Aug 19, 2018

@bors r+ rollup

@bors
Copy link
Contributor

bors commented Aug 19, 2018

📌 Commit 2fae6e2ae114cd9e7dc1a1d4e415eab57c19ca3e has been approved by dtolnay

@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 19, 2018
@rust-highfive
Copy link
Collaborator

The job x86_64-gnu-llvm-5.0 of 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.
[00:44:35] ....................................................................................................
[00:44:37] ....................................................................................................
[00:44:40] ....................................................................................................
[00:44:44] ........i...........................................................................................
[00:44:48] ..................................................................................F...........i.....
[00:44:51] ..i.................................................................................................
[00:44:56] ....................................................................................................
[00:44:58] ....................................................................................................
[00:45:01] ....................................................................................................
[00:45:03] ....................................................................................................
---
[00:46:12] ....................................................................................................
[00:46:16] .......................................i............................................................
[00:46:19] ....................................................................................................
[00:46:22] ....................................................................................................
":[{"file_name":"/checkout/src/test/ui/consts/const-size_of-cycle.rs","byte_start":0,"byte_end":0,"line_start":1,"line_end":1,"column_start":1,"column_end":1,"is_primary":true,"text":[{"text":"// Copyright 2017 The Rust Project Developers. See the COPYRIGHT","highlight_start":1,"highlight_end":1}],"label":null,"suggested_replacement":null,"suggestion_applicability":null,"expansion":null}],"children":[{"message":"...which requires normalizing `ParamEnvAnd { param_env: ParamEnv { caller_bounds: [], reveal: All }, value: [u8; _] }`...","code":null,"level":"note","spans":[{"file_name":"/checkout/src/test/ui/consts/const-size_of-cycle.rs","byte_start":0,"byte_end":0,"line_start":1,"line_end":1,"column_start":1,"column_end":1,"is_primary":true,"text":[{"text":"// Copyright 2017 The Rust Project Developers. See the COPYRIGHT","highlight_start":1,"highlight_end":1}],"label":null,"suggested_replacement":null,"suggestion_applicability":null,"expansion":null}],"children":[],"rendered":null},{"message":"...which requires const-evaluating `Foo::bytes::{{constant}}`...","code":null,"level":"note","spans":[{"file_name":"/checkout/src/libcore/mem.rs","byte_start":9648,"byte_end":9674,"line_start":291,"line_end":291,"column_start":14,"column_end":40,"is_primary":true,"text":[{"text":"    unsafe { intrinsics::size_of::<T>() }","highlight_start":14,"highlight_end":40}],"label":null,"suggested_replacement":null,"suggestion_applicability":null,"expansion":null}],"children":[],"rendered":null},{"message":"...which again requires computing layout of `Foo`, completing the cycle","code":null,"level":"note","spans":[],"children":[],"rendered":null},{"

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)

@kornelski
Copy link
Contributor Author

The build failure seems to be from unrelated commit.

@kennytm
Copy link
Member

kennytm commented Aug 21, 2018

@bors r-

This PR caused UI test failure in #53530 (comment). That UI test contains some line numbers from libcore/mem.rs which this PR changed.

@bors bors 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-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Aug 21, 2018
@dtolnay
Copy link
Member

dtolnay commented Aug 21, 2018

I filed #53547 to fix the brittle test. For now @kornelski 😭 it looks like you will need to find the new line number and update const-size_of-cycle.stderr in this PR. (Or fix #53547 yourself if you are up for it?)

@kornelski
Copy link
Contributor Author

Fix for the other test is now merged. I've rebased this.

@dtolnay
Copy link
Member

dtolnay commented Aug 23, 2018

@bors r+ rollup

@bors
Copy link
Contributor

bors commented Aug 23, 2018

📌 Commit e7709b3 has been approved by dtolnay

@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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Aug 23, 2018
kennytm added a commit to kennytm/rust that referenced this pull request Aug 24, 2018
Discourage overuse of mem::forget

Some uses of `mem::forget` have been replaced by better methods of `Box`, so I've removed obsoleted use-cases from these docs.

I've removed emphasis on leaking, because it's not obvious `mem::forget` does not guarantee leaking of memory: memory of stack-allocated objects and values partially moved out of `Box` will still be freed. That's a potential error when used to pass objects to FFI, so it's better to direct users to `Box::into_raw` instead.
bors added a commit that referenced this pull request Aug 24, 2018
Rollup of 16 pull requests

Successful merges:

 - #53311 (Window Mutex: Document that we properly initialize the SRWLock)
 - #53503 (Discourage overuse of mem::forget)
 - #53545 (Fix #50865: ICE on impl-trait returning functions reaching private items)
 - #53559 (add macro check for lint)
 - #53562 (Lament the invincibility of the Turbofish)
 - #53563 (use String::new() instead of String::from(""), "".to_string(), "".to_owned() or "".into())
 - #53592 (docs: minor stylistic changes to str/string docs)
 - #53594 (Update RELEASES.md to include clippy-preview)
 - #53600 (Fix a grammatical mistake in "expected generic arguments" errors)
 - #53614 (update nomicon and book)
 - #53617 (tidy: Stop requiring a license header)
 - #53618 (Add missing fmt examples)
 - #53636 (Prefer `.nth(n)` over `.skip(n).next()`.)
 - #53644 (Use SmallVec for SmallCStr)
 - #53664 (Remove unnecessary closure in rustc_mir/build/mod.rs)
 - #53666 (Added rustc_codegen_llvm to compiler documentation.)
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.

5 participants