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

Minor text improvements #428

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Conversation

proski
Copy link

@proski proski commented Feb 24, 2025

Trying to clarify some parts of the book, whatever sounded unclear to me.

It internally manages exactly one copy of the data. Invoking `.clone()` on `Rc`
produces a new `Rc` instance, which points to the same data as the source `Rc`,
while increasing a reference count. The same applies to `Arc`, the thread-safe
counterpart of `Rc`.
Copy link
Author

Choose a reason for hiding this comment

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

It sounded like the use of heap is specific to Arc. I believe the use of heap is irrelevant here. I combined the best parts of Rc and Arc paragraphs and mentioned Arc at the end.

[2](https://rust-lang.github.io/rust-clippy/master/index.html#clone_on_copy),
[3](https://rust-lang.github.io/rust-clippy/master/index.html#map_clone) or
[4](https://rust-lang.github.io/rust-clippy/master/index.html#clone_double_ref).
some cases in which `.clone()` is not necessary.
Copy link
Author

Choose a reason for hiding this comment

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

The last link is broken. I can see old mentions of clone_double_ref, perhaps it was removed. More importantly, clippy is unlikely to recommend removing .clone() if the compiler requires it (which is the premise of this page). But clippy can find something else that could be improved, which would in turn make .clone() unnecessary (e.g. clippy might suggest passing arguments to a function by reference so the value is not consumed). No need to focus on clone-specific lints.

@@ -106,6 +106,6 @@ certain that there will be more deprecated APIs in the future.
`rustc --help` for a general list of options
- [rust-clippy] is a collection of lints for better Rust code

[rust-clippy]: https://github.com/Manishearth/rust-clippy
[rust-clippy]: https://github.com/rust-lang/rust-clippy
Copy link
Author

Choose a reason for hiding this comment

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

The link was to a fork of the official rust-clippy repository. I don't see anything interesting in that fork. Let's link to the original.

@@ -238,7 +238,7 @@ improve in the future.
This pattern is used throughout the standard library:

- `Vec<u8>` can be cast from a String, unlike every other type of `Vec<T>`.[^1]
- They can also be cast into a binary heap, but only if they contain a type that
- Iterators can be cast into a binary heap, but only if they contain a type that
Copy link
Author

Choose a reason for hiding this comment

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

I don't know who "they" are. Looking at the link, it's iterators.

@@ -275,7 +275,7 @@ struct TestStruct {
generate_visitor!(TestStruct);
```

Or do they?
But let's actually try that approach.
Copy link
Author

Choose a reason for hiding this comment

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

Another unclear "they".

@@ -153,7 +153,7 @@ optimizations that C code cannot attain. Being denied access to certain
shortcuts is the price Rust programmers need to pay.

[^1]: For the C programmers out there scratching their heads, the iterator need
not be read *during* this code cause the UB. The exclusivity rule also enables
not be read *during* this code to cause the UB. The exclusivity rule also enables
Copy link
Author

@proski proski Feb 24, 2025

Choose a reason for hiding this comment

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

I hope I understood that correctly. It might be better to move this text from the footnote and expand it for clarity.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant