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

docs: clarify explicitly freeing heap allocated memory #117563

Merged
merged 1 commit into from
Dec 6, 2023
Merged

docs: clarify explicitly freeing heap allocated memory #117563

merged 1 commit into from
Dec 6, 2023

Conversation

0xalpharush
Copy link
Contributor

The documentation for Box::into_raw didn't mention drop and wondered if I was doing something wrong. Based off this, I think it's helpful to include the more concise yet explicit way to free heap allocated memory. This is my first rust PR and I went through https://std-dev-guide.rust-lang.org/development/, but let me know if I missed something :)

@rustbot
Copy link
Collaborator

rustbot commented Nov 4, 2023

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @m-ou-se (or someone else) soon.

Please see the contribution instructions for more information. Namely, in order to ensure the minimum review times lag, PR authors and assigned reviewers should ensure that the review label (S-waiting-on-review and S-waiting-on-author) stays updated, invoking these commands when appropriate:

  • @rustbot author: the review is finished, PR author should check the comments and take action accordingly
  • @rustbot review: the author is ready for a review, this PR will be queued again in the reviewer's queue

@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 Nov 4, 2023
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@workingjubilee
Copy link
Member

@joboet can you elaborate on "whether it should be the other way around"?

@joboet
Copy link
Member

joboet commented Dec 5, 2023 •
edited
Loading

(for context: we were discussing this PR and a few others on Zulip)

Mentioning the current example as an alternative should people who need to use the allocator themselves at ease, but IMHO most people should use

drop(Box::from_raw(...))

since it avoids the pitfalls associated with the other version (thinking about it, there is actually a bug in it: if the destructor panics, the memory gets leaked). So the Box::from_raw version should be the main example.

EDIT: Oh wait, that example is already given above. Then perhaps modifying the first example would make more sense?

@workingjubilee
Copy link
Member

Hmm.

I'm going to say "this seems wanted and I'm happy to review anyone's suggestions re: reorganizing it."

@bors r+ rollup

@bors
Copy link
Contributor

bors commented Dec 5, 2023

📌 Commit c7d8c65 has been approved by workingjubilee

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

Rollup of 7 pull requests

Successful merges:

 - rust-lang#116496 (Provide context when `?` can't be called because of `Result<_, E>`)
 - rust-lang#117563 (docs: clarify explicitly freeing heap allocated memory)
 - rust-lang#117874 (`riscv32` platform support)
 - rust-lang#118516 (Add ADT variant infomation to StableMIR and finish implementing TyKind::internal())
 - rust-lang#118650 (add comment about keeping flags in sync between bootstrap.py and bootstrap.rs)
 - rust-lang#118664 (docs: remove rust-lang#110800 from release notes)
 - rust-lang#118669 (library: fix comment about const assert in win api)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 49d6594 into rust-lang:master Dec 6, 2023
11 checks passed
@rustbot rustbot added this to the 1.76.0 milestone Dec 6, 2023
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Dec 6, 2023
@rust-timer
Unrolled build for rust-lang#117563
f7d875a 
Rollup merge of rust-lang#117563 - 0xalpharush:docs/into-raw, r=workingjubilee

docs: clarify explicitly freeing heap allocated memory

The documentation for `Box::into_raw` didn't mention `drop` and wondered if I was doing something wrong. Based off [this](https://stackoverflow.com/questions/75441199/rust-how-do-i-correctly-free-heap-allocated-memory), I think it's helpful to include the more concise yet explicit way to free heap allocated memory. This is my first rust PR and I went through https://std-dev-guide.rust-lang.org/development/, but let me know if I missed something :)
@0xalpharush 0xalpharush deleted the docs/into-raw branch February 7, 2024 19:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@m-ou-se m-ou-se

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.76.0
Development

Successfully merging this pull request may close these issues.
7 participants
@0xalpharush @rustbot @rust-log-analyzer @workingjubilee @joboet @bors @m-ou-se