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

Std module docs improvements #93162

Merged
merged 1 commit into from
Aug 22, 2022
Merged

Conversation

camsteffen
Copy link
Contributor

My primary goal is to create a cleaner separation between primitive types and primitive type helper modules (fixes #92777). I also changed a few header lines in other top-level std modules (seen at https://doc.rust-lang.org/std/) for consistency.

Some conventions used/established:

  • "The `Box<T>` type for heap allocation." - if a module mainly provides a single type, name it and summarize its purpose in the module header
  • "Utilities for the _ primitive type." - this wording is used for the header of helper modules
  • Documentation for primitive types themselves are removed from helper modules
  • provided-by-core functionality of primitive types is documented in the primitive type instead of the helper module (such as the "Iteration" section in the slice docs)

I wonder if some content in std::ptr should be in pointer but I did not address this.

@rust-highfive
Copy link
Collaborator

r? @m-ou-se

(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 Jan 21, 2022
@m-ou-se
Copy link
Member

m-ou-se commented Jan 22, 2022

It makes me super happy to see someone working on documentation consistency. I'll try to make the time to review this soon.

@m-ou-se m-ou-se added the T-libs Relevant to the library team, which will review and decide on the PR/issue. label Jan 22, 2022
Copy link
Member

@m-ou-se m-ou-se left a comment

Choose a reason for hiding this comment

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

This is great. Thanks for doing this.

I have a few small comments, but all of them are basically comments about the old documentation that you moved, so not really comments on your changes. If you feel like addressing them, go ahead. But r=me either way. :)

/// ```
///
/// They are `'static` because they're stored directly in the final binary, and
/// so will be valid for the `'static` duration.
/// Here we have declared a string literal, also known as a string slice.
Copy link
Member

Choose a reason for hiding this comment

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

While every string literal is a string slice, not every string slice is a literal, so let's avoid implying they are the same thing. (A 'string slice' could also refer to (a part of) a String, for example.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Suggested change
/// Here we have declared a string literal, also known as a string slice.
/// Here we have declared a string slice initialized with a string literal.

#[stable(feature = "rust1", since = "1.0.0")]
mod prim_slice {}

#[doc(primitive = "str")]
//
/// String slices.
/// Unicode string slices.
Copy link
Member

Choose a reason for hiding this comment

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

I'd be careful with using the term 'unicode string'. That's used in some other languages/on other platforms to refer to [u32] or [u16] string representations. You could mention UTF-8 specifically, but that's already mentioned below and might be a bit too much detail for the first line.

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 could just undo this change. Also is "slices" a bit over-technical or confusing? I'm not how sure "slice" applies to a const &'static str for example. How about "The primitive string type"?

Copy link
Member

Choose a reason for hiding this comment

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

"Slices" might be important to not cause confusion with the String type.

I'm not how sure "slice" applies to a const &'static str for example.

That's still a pointer+size to a 'slice' of the program (where the string is stored). But I suppose the main point is that if you take any substring, you still have the same type.

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'll just revert this.

Comment on lines 1 to 2
//! The `Any` trait enables dynamic typing of any `'static` type through runtime
//! reflection.
Copy link
Member

Choose a reason for hiding this comment

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

This module also provides TypeId and type_name. While related to Any, they're also useful without directly using that trait.

Not entirely sure how to summarize these together in one line though. Something about type erasure or dynamic typing or reflection maybe?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

How about

Utilities for dynamic typing or working with any type.

supposing Any and TypeId falls under "dynamic typing" and type_name is a util that works with any type.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah that could work. Or maybe "Utilities related to type erasure."? Either way is fine, as long as it doesn't imply it's only the Any type.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Currently landed on "Utilities for dynamic typing or type reflection." I feel that "erasure" is a sub-concept of "dynamic typing", and TypeId and type_name are both utilities for "reflection", so that word is good to include.

Comment on lines 749 to 829
/// ```
/// let numbers = &[0, 1, 2];
/// for n in numbers {
/// println!("{} is a number!", n);
/// }
/// ```
///
/// The mutable slice yields mutable references to the elements:
///
/// ```
/// let mut scores = [7, 8, 9];
/// for score in &mut scores[..] {
/// *score += 1;
/// }
/// ```
Copy link
Member

Choose a reason for hiding this comment

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

Both of these examples work with and without [..], but only the mut version uses that now. The first one now iterates over a reference to an array, not over the (unsized) slice.

Maybe these should coerce the type explicitly to a slice, and then iterate over that without any [..]?

Suggested change
/// ```
/// let numbers = &[0, 1, 2];
/// for n in numbers {
/// println!("{} is a number!", n);
/// }
/// ```
///
/// The mutable slice yields mutable references to the elements:
///
/// ```
/// let mut scores = [7, 8, 9];
/// for score in &mut scores[..] {
/// *score += 1;
/// }
/// ```
/// ```
/// let numbers: &[i32] = &[0, 1, 2];
/// for n in numbers {
/// println!("{} is a number!", n);
/// }
/// ```
///
/// The mutable slice yields mutable references to the elements:
///
/// ```
/// let scores: &mut [i32] = &mut [7, 8, 9];
/// for score in scores {
/// *score += 1;
/// }
/// ```

@m-ou-se m-ou-se 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 Feb 7, 2022
@JohnCSimon
Copy link
Member

ping from triage:
@camsteffen
can you address the comments from m-ou-se?

FYI: when a PR is ready for review, post a message containing
@rustbot ready to switch the PR to S-waiting-on-review so the PR appears in the reviewer's backlog.

@camsteffen
Copy link
Contributor Author

My bad. Waiting for feedback.

@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 Mar 6, 2022
@m-ou-se m-ou-se 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 Mar 9, 2022
@bors
Copy link
Contributor

bors commented Mar 11, 2022

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

@camsteffen
Copy link
Contributor Author

@rustbot ready

Rebased and addressed comments.

@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 Apr 30, 2022
@camsteffen
Copy link
Contributor Author

Threw in a fix for variable capturing in tidy_error!.

@bors
Copy link
Contributor

bors commented May 26, 2022

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

@camsteffen
Copy link
Contributor Author

camsteffen commented May 27, 2022

Rebased. I'm not sure if I'm conflicting with the intent of #96033 which seems to make std::error into a page about general error handling best practices.

@camsteffen camsteffen requested a review from m-ou-se May 27, 2022 00:11
@bors
Copy link
Contributor

bors commented Jun 10, 2022

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

@JohnCSimon
Copy link
Member

ping from triage:
@camsteffen this has a merge conflict

@rustbot

This comment was marked as resolved.

@camsteffen
Copy link
Contributor Author

Rebased. I decided to bulldoze the header line added to core::any in #91970 in order to keep it short.

library/alloc/src/slice.rs Outdated Show resolved Hide resolved
library/core/src/primitive_docs.rs Outdated Show resolved Hide resolved
library/core/src/primitive_docs.rs Outdated Show resolved Hide resolved
@Mark-Simulacrum Mark-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, 2022
@camsteffen
Copy link
Contributor Author

Addressed @Mark-Simulacrum's comments.

@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 Aug 20, 2022
@Mark-Simulacrum
Copy link
Member

r=me with commits squashed and the unrelated tidy change dropped.

@camsteffen
Copy link
Contributor Author

@bors r=Mark-Simulacrum

@bors
Copy link
Contributor

bors commented Aug 20, 2022

📌 Commit 17ddcb4 has been approved by Mark-Simulacrum

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 Aug 20, 2022
bors added a commit to rust-lang-ci/rust that referenced this pull request Aug 22, 2022
Rollup of 5 pull requests

Successful merges:

 - rust-lang#93162 (Std module docs improvements)
 - rust-lang#99386 (Add tests that check `Vec::retain` predicate execution order.)
 - rust-lang#99915 (Recover keywords in trait bounds)
 - rust-lang#100694 (Migrate rustc_ast_passes diagnostics to `SessionDiagnostic` and translatable messages (first part))
 - rust-lang#100757 (Catch overflow early)

Failed merges:

 - rust-lang#99917 (Move Error trait into core)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit a4950ef into rust-lang:master Aug 22, 2022
@rustbot rustbot added this to the 1.65.0 milestone Aug 22, 2022
@yaahc
Copy link
Member

yaahc commented Aug 22, 2022

Rebased. I'm not sure if I'm conflicting with the intent of #96033 which seems to make std::error into a page about general error handling best practices.

Looks like the answer is yes 😅, I'm going to go ahead and revert the change you made to the module description since I intend to have more than just the Error trait in the std::error module, such as Chain and Report types.

@camsteffen camsteffen deleted the std-prim-docs branch August 22, 2022 22:52
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. T-libs Relevant to the library team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Docs for primitive type helper modules should stay in their lane
8 participants