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

Clarify the difference between get_mut and into_mut for OccupiedEntry #49743

Closed

Conversation

Phlosioneer
Copy link
Contributor

@Phlosioneer Phlosioneer commented Apr 6, 2018

The examples for both hash_map::OccupiedEntry::get_mut and
hash_map::OccupiedEntry::into_mut were almost identical. This led
to some confusion over the difference, namely why you would ever
use get_mut when into_mut gives a longer lifetime. Reddit thread:
https://www.reddit.com/r/rust/comments/8a5swr/why_does_hashmaps

This commit adds two lines and a comment to the example, to show
that the entry object can be re-used after calling get_mut.

I'm open to alternate ways to modify these examples.

The examples for both hash_map::OccupiedEntry::get_mut and
hash_map::OccupiedEntry::into_mut were almost identical. This led
to some confusion over the difference, namely why you would ever
use get_mut when into_mut gives alonger lifetime. Reddit thread:
https://www.reddit.com/r/rust/comments/8a5swr/why_does_hashmaps

This commit adds two lines and a comment to the example, to show
that the entry object can be re-used after calling get_mut.
@rust-highfive
Copy link
Contributor

r? @BurntSushi

(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 Apr 6, 2018
@BurntSushi
Copy link
Member

Is it worth perhaps explicitly calling out the difference between into_mut and get_mut in the docs? It seems like the comments in the reddit thread could be re-molded into a couple short sentences in the docs.

Thanks for taking the time to improve this. :-)

@Phlosioneer
Copy link
Contributor Author

Sure. I just don't know what to say exactly - there's a difference between giving a rough guesstimation of the difference on reddit and encoding information into the docs.

@Phlosioneer
Copy link
Contributor Author

(See also my separate but related issue #49745)

@kennytm kennytm added the A-docs Area: Documentation for any part of the project, including the compiler, standard library, and tools label Apr 14, 2018
@pietroalbini
Copy link
Member

Ping from triage! Can @BurntSushi (or someone else from @rust-lang/docs) review this PR?

@frewsxcv
Copy link
Member

I'm okay with the changes as-is, though in my opinion, I think there's still room for a statement outside the doc example making a direct comparison between both methods.

For example, for get_mut:

     /// Gets a mutable reference to the value in the entry.
     ///
+    /// Unlike the [`into_mut`] method, `get_mut` should be used when you ______.
     ///
     /// # Examples

or

     /// Gets a mutable reference to the value in the entry.
     ///
+    /// If you need to ______ see the [`into_mut`] method.
     ///
     /// # Examples

The challenge here is succinctly filling in the ______ blanks.

Will think about this for a bit and maybe give other docs team members some time to respond. Will revisit in a few days and approve if there's not any other activity.

@GuillaumeGomez
Copy link
Member

Hum not much to say. Just wondering what @frewsxcv will find out. :D

@pietroalbini
Copy link
Member

Ping from triage! What's the status of this PR?

@Phlosioneer
Copy link
Contributor Author

I'm going through stuff and haven't had the time to finish this yet. It's not forgotten, though.

@shepmaster shepmaster 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 May 6, 2018
@pietroalbini
Copy link
Member

Ping from triage @Phlosioneer! Will you have time to work on this again in the future?

@frewsxcv
Copy link
Member

I'm not confident with the wording of these, but:

For into_mut, maybe add:

If you need multiple references to the `OccupiedEntry`, see `get_mut`.

And for get_mut, maybe:

If you need a reference to the `OccupiedEntry` which may outlive the destruction of the `Entry` value, see `into_mut`.

Also if it's easy for you, please also make these changes to the btree_map docs:

@pietroalbini
Copy link
Member

Assigning to someone else from libs. r? @dtolnay

@rust-highfive rust-highfive assigned dtolnay and unassigned BurntSushi May 21, 2018
Copy link
Member

@dtolnay dtolnay left a comment

Choose a reason for hiding this comment

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

I agree with the review in #49743 (comment).

@dtolnay dtolnay assigned frewsxcv and unassigned dtolnay May 21, 2018
@pietroalbini
Copy link
Member

Ping from triage @Phlosioneer! It's been a while since we heard from you, will you have time to work on this again?

@frewsxcv
Copy link
Member

frewsxcv commented Jun 2, 2018

In an attempt to keep this moving, I just opened a new pull request which cherry picks these two picks and adds in my suggested changes: #51312 . Hope that's alright @Phlosioneer!

@frewsxcv frewsxcv closed this Jun 2, 2018
@Phlosioneer
Copy link
Contributor Author

That's okay. I've been unable to devote time to rustc lately.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-docs Area: Documentation for any part of the project, including the compiler, standard library, and tools S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants