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

Add doc aliases for memory allocations #79233

Merged
merged 1 commit into from Jan 23, 2021

Conversation

yoshuawuyts
Copy link
Member

This patch adds doc aliases for various C allocation functions, making it possible to search for the C-equivalent of a function and finding the (safe) Rust counterpart:

  • Vec::with_capacity / Box::new / vec! -> alloc + malloc, allocates memory
  • Box::new_zeroed -> calloc, allocates zeroed-out memory
  • Vec::{reserve,reserve_exact,try_reserve_exact,shrink_to_fit,shrink_to} -> realloc, reallocates a previously allocated slice of memory

It's worth noting that Vec::new does not allocate, so we don't link to it. Instead people are probably looking for Vec::with_capacity or vec!. I hope this will allow people comfortable with the system allocation APIs to make it easier to find what they may be looking for.

Thanks!

@rust-highfive
Copy link
Collaborator

r? @joshtriplett

(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 Nov 20, 2020
@joshtriplett
Copy link
Member

joshtriplett commented Nov 20, 2020

This is a great idea!

For calloc, it isn't just used to allocate zeroed memory; it's also used to allocate arrays of structures, since it takes a number of elements and a size for each element. So, I think it'd be appropriate for calloc to lead to Vec::with_capacity as well.

Also consider aliases for alloc, calloc, and malloc on String::with_capacity.

I'd love to see PRs like this for other common C functions as well, such as the str* family of string functions, and the mem* family of memory functions. For instance, strcmp and memcmp could point to the documentation for Ord and Eq (the portions that mention the corresponding operators). (Those can be separate PRs; the only changes I'm requesting in this PR are the additional aliases for String::with_capacity and the additional alias for calloc.)

library/alloc/src/vec.rs Outdated Show resolved Hide resolved
@GuillaumeGomez GuillaumeGomez added A-doc-alias Area: #[doc(alias)] T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. labels Dec 4, 2020
@yoshuawuyts
Copy link
Member Author

I've updated with @joshtriplett's feedback.

@Manishearth @GuillaumeGomez did you resolve the conversation in #79211? -- I'm tempted to tag both libs and docs on this for review, but wanted to check before doing so.

@GuillaumeGomez GuillaumeGomez removed the T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. label Dec 18, 2020
@Manishearth
Copy link
Member

@yoshuawuyts yeah mostly "ping both @GuillaumeGomez and the libs team for doc changes". Hopefully they can form a libs-docs team eventually.

Feel free to tag the docs team, it's just kinda defunct right now and you'd be tagging more people than you need.

@GuillaumeGomez
Copy link
Member

No need. Just doc-alias is enough (so we can keep track of the usage of the feature in case we need to debug something).

@yoshuawuyts
Copy link
Member Author

Got it, thanks heaps!

@bors
Copy link
Contributor

bors commented Dec 31, 2020

The latest upstream changes (presumably #80530) made this pull request unmergeable. Please resolve the merge conflicts.

@crlf0710
Copy link
Member

Triage: there's merge conflicts now.

@yoshuawuyts
Copy link
Member Author

Oh dang, this is because of the new allocator_api methods isn't it? I'm out of time to address these changes today, but I'll try and squeeze them into next week Friday.

@yoshuawuyts
Copy link
Member Author

Resolved the merge conflicts; a simple rebase was enough. This is again ready for review!

library/alloc/src/string.rs Outdated Show resolved Hide resolved
library/alloc/src/vec/mod.rs Outdated Show resolved Hide resolved
@@ -828,6 +833,7 @@ impl<T, A: Allocator> Vec<T, A> {
/// }
/// # process_data(&[1, 2, 3]).expect("why is the test harness OOMing on 12 bytes?");
/// ```
#[doc(alias = "realloc")]
Copy link
Member

Choose a reason for hiding this comment

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

Even though this realloc alias makes sense, having so many of it is kinda killing its purpose, no?

Copy link
Member Author

Choose a reason for hiding this comment

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

Rust has no canonical "realloc" function; instead it's split up between various different methods which perform specific kinds of reallocations. Someone searching for "realloc" may not be aware of this, and sharing the various ways in which vectors can be reallocated may be helpful.

Conversely an experienced Rust programmer may not recall the exact name of a specific reallocation they're trying to perform (or may want to validate whether a vec method indeed reallocates) and searching for "realloc" may provide them with an answer.

Copy link
Member

Choose a reason for hiding this comment

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

Fine by me, let's give it a try like this then!

- Vec::with_capacity / Box::new -> alloc + malloc
- Box::new_zeroed -> calloc
- Vec::{reserve,reserve_exact,try_reserve_exact,shrink_to_fit,shrink_to} -> realloc
@GuillaumeGomez
Copy link
Member

Thanks!

@bors: r+

@bors
Copy link
Contributor

bors commented Jan 22, 2021

📌 Commit 7d10238 has been approved by GuillaumeGomez

@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 Jan 22, 2021
@bors
Copy link
Contributor

bors commented Jan 22, 2021

Testing commit 7d10238 with merge 34b3d41...

@bors
Copy link
Contributor

bors commented Jan 23, 2021

☀️ Test successful - checks-actions
Approved by: GuillaumeGomez
Pushing 34b3d41 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors label Jan 23, 2021
@bors bors merged commit 34b3d41 into rust-lang:master Jan 23, 2021
1 check passed
@rustbot rustbot added this to the 1.51.0 milestone Jan 23, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-doc-alias Area: #[doc(alias)] merged-by-bors This PR was explicitly merged by bors 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.

None yet

8 participants