Doc:std::convert explicitely list generic impls #30901
Conversation
Also add a note about the necessary simplicity of the conversion. Related issue: rust-lang#29349
|
Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @steveklabnik (or someone else) soon. If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes. Please see the contribution instructions for more information. |
|
As a side note, I wrote nothing about #29701 as it wasn't crystal clear what has to be written about it (library writer has to document exactly when and how to use conversions ? Library user should not trust conversions for some contexts ?). I am happy to change or reword anything, add references, links, or topics. |
| @@ -17,6 +17,26 @@ | |||
| //! Like many traits, these are often used as bounds for generic functions, to | |||
| //! support arguments of multiple types. | |||
| //! | |||
| //! - Use `as` for reference-to-reference conversions | |||
There was a problem hiding this comment.
as is broader than this, actually. it's for any kind of basic cast.
There was a problem hiding this comment.
The RFC which introduced them doesn't agree (ref) and has a nice rationale about it. If the rfc is still right, maybe explain further the motivation ?
There was a problem hiding this comment.
This is about the as keyword, not the As* traits, no?
There was a problem hiding this comment.
So yes, I think that all we need is proper capitalization/style here, to disambiguate from the keywords.
| @@ -30,6 +50,10 @@ use marker::Sized; | |||
| /// | |||
| /// [book]: ../../book/borrow-and-asref.html | |||
| /// | |||
| /// **Note:** these traits are for trivial conversion. **They must not fail**. If | |||
There was a problem hiding this comment.
It may be worth it to call out here that there's no technical reason why they should not fail right now, it's just idiomatic that they should never fail.
There was a problem hiding this comment.
Do you mean that a panic could be acceptable in this case or do you mean that implementing Into<Option<T>> is not idiomatic ?
There was a problem hiding this comment.
Ah no panics are still "unacceptable" but not in the sense that something like unsafety will arise. It's just unidiomatic and can lead to bugs, so it's rather strongly recommended that conversions don't panic.
The wording currently implies to me that something very bad will happen if a panic happens during a conversions, but it should be pretty normal in terms of the outcome (the code just panics). It's just the standard worry about a function panicking.
| @@ -17,6 +17,24 @@ | |||
| //! Like many traits, these are often used as bounds for generic functions, to | |||
| //! support arguments of multiple types. | |||
| //! | |||
| //! - Use `as` for reference-to-reference conversions | |||
| //! - Use `into` when you want to consume the value | |||
|
Just those last bits of capitalization and r=me |
|
(Sorry for not noticing this was updated, GitHub doesn't notify when a new commit is pushed :/ |
|
I missed the github notification, sorry. Here is a disambiguation, it was indeed confusing. |
|
@bors: r+ rollup Thanks a ton! |
|
📌 Commit a0cd465 has been approved by |
Also add a note about the necessary simplicity of the conversion. Related issue: #29349 r? @steveklabnik
Also add a note about the necessary simplicity of the conversion.
Related issue: #29349
r? @steveklabnik