-
Notifications
You must be signed in to change notification settings - Fork 12.9k
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
Document From
implementations
#51753
Document From
implementations
#51753
Conversation
This comment has been minimized.
This comment has been minimized.
Restarted the Travis CI build, the failure was caused by the crates.io outage. |
Assigning someone from the libs team. r? @sfackler |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
First of all @gruberb, thanks a lot!
I marked a couple of things. They are mostly the things why I think this is a good exercise :)!
I've also got things to discuss: I think it would be good to call pure type conversions "free", while things that go from one management structure to another (e.g. OsString -> OsStr) the way you are currently doing things (in-place).
@steveklabnik would you be up for some language clarification?
@@ -1419,20 +1423,28 @@ impl<'a, T: ?Sized + AsRef<OsStr>> From<&'a T> for PathBuf { | |||
|
|||
#[stable(feature = "rust1", since = "1.0.0")] | |||
impl From<OsString> for PathBuf { | |||
/// Converts a `OsString` into a `PathBuf`. | |||
/// This conversion copies the data. | |||
/// This conversion does allocate memory. | |||
fn from(s: OsString) -> PathBuf { | |||
PathBuf { inner: s } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Interestingly, this does not allocate memory. This is free.
PathBuf
is a wrapper around OsString
. If you have a struct
with just one field, its memory structure is the same as the field itself. The Compiler "sees" through this and knows that turning OsString into a PathBuf is effectively a no-op.
See https://godbolt.org/g/1PBCTY (using String
, as an example, but the principle is the same). If you have a close look at the blue and orange part, this just sets up for returning the previously allocated String
. It doesn't even call the implementation!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks a lot for the explanation, was a while since I looked at Assembler. Do you have a general set of tools you use for investigating memory allocation? I sort of did all over the place with no real plan.
src/libstd/path.rs
Outdated
@@ -1419,20 +1423,28 @@ impl<'a, T: ?Sized + AsRef<OsStr>> From<&'a T> for PathBuf { | |||
|
|||
#[stable(feature = "rust1", since = "1.0.0")] | |||
impl From<OsString> for PathBuf { | |||
/// Converts a `OsString` into a `PathBuf`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure if we need this sentence, but let's hear opinions. It's basically restating the code already says.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The format is generally "summary sentence, then other stuff". If you want other stuff, you need a summary. I'm not sure how else you'd summarize this. TL;DR it's fine.
src/libstd/path.rs
Outdated
fn from(s: OsString) -> PathBuf { | ||
PathBuf { inner: s } | ||
} | ||
} | ||
|
||
#[stable(feature = "from_path_buf_for_os_string", since = "1.14.0")] | ||
impl From<PathBuf> for OsString { | ||
/// Converts a `PathBuf` into a `OsString`. | ||
/// This conversion copies the data. | ||
/// This conversion does allocate memory. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here, but the other way around: Unwrapping a wrapper is free!
src/libstd/path.rs
Outdated
fn from(path_buf : PathBuf) -> OsString { | ||
path_buf.inner | ||
} | ||
} | ||
|
||
#[stable(feature = "rust1", since = "1.0.0")] | ||
impl From<String> for PathBuf { | ||
/// Converts a `String` into a `PathBuf`. | ||
/// This conversion does not allocate memory |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Have you investigated this for all platforms? I just did a cursory review, and it's true for Unix and Windows.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It should be the same for all of them.
src/libstd/path.rs
Outdated
/// Converts a `PathBuf` into a `Arc<Path>`. | ||
/// This conversion happens in place. | ||
/// This conversion does not allocate memory. | ||
/// This function is unsafe. Data can't be moved from this reference. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How is this function unsafe? It isn't marked as such.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hm, I was thinking because of line 1542. It calls internally an unsafe
function. But I guess if the calling method is safe, then it doesn't matter if it calls unsafe
internally?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, Rust practice is to assume that the unsafety is contained in the block. Calling the function unsafe declares that people need to fulfil certain conditions before calling the function to ensure it can be safely called.
The reasoning behind that is that otherwise, everything would be transitively unsafe.
src/libstd/path.rs
Outdated
/// Converts a `PathBuf` into a `Arc<Path>`. | ||
/// This conversion happens in place. | ||
/// This conversion does not allocate memory. | ||
/// This function is unsafe. Data can't be moved from this reference. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here.
src/libstd/path.rs
Outdated
/// Converts a `PathBuf` into a `Rc<Path>`. | ||
/// This conversion happens in place. | ||
/// This conversion does not allocate memory. | ||
/// This function is unsafe. Data can't be moved from this reference. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here.
Ping from triage @sfackler , will you have time to review this? |
I'll defer to the docs team. |
Ping from triage @steveklabnik @rust-lang/docs guys, this PR needs your review. |
src/libstd/path.rs
Outdated
fn from(boxed: Box<Path>) -> PathBuf { | ||
boxed.into_path_buf() | ||
} | ||
} | ||
|
||
#[stable(feature = "box_from_path_buf", since = "1.20.0")] | ||
impl From<PathBuf> for Box<Path> { | ||
/// Converts a `PathBuf` into a `Box<Path>`. | ||
/// This conversion does not allocate memory |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Question for @rust-lang/docs: should we start specifying allocation behavior in the docs, like this line? I forgot if we've discussed this already in the past. Also FWIW, doesn't look like we specify allocation behavior in the docs for the underlying into_boxed_path
method.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this would be a good idea. Rust is meant to be memory efficient, therefore, having allocation behavior precisions seems like a must have.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think in many cases we'll need to check in with @rust-lang/libs but in obvious cases like this, it's meant to do so and so should be.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Convertion from an extensible container like PathBuf
to a fixed size like Box<Path>
doesn't necessarily explicitly allocate memory but it does use shrink_to_fit
which attempts to shrink the allocation of PathBuf
to precisely match the Path
in question. Most of the time this doesn't allocate memory, but to be safe it's probably best to avoid accidentally providing a guarantee that this doesn't allocate.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@alexcrichton Should we add this in the docs or remove them all together?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nah I think having them is fine so long as it's clear that currently it's unlikely to reallocate/allocate memory, but this isn't necessarily guaranteed in the future
src/libstd/path.rs
Outdated
@@ -1529,6 +1542,9 @@ impl From<PathBuf> for Arc<Path> { | |||
|
|||
#[stable(feature = "shared_from_slice2", since = "1.24.0")] | |||
impl<'a> From<&'a Path> for Arc<Path> { | |||
/// Converts a `PathBuf` into a `Arc<Path>`. | |||
/// This conversion happens in place. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we need to specify in the docs which functions are marked inline, rustdoc already renders the #[inline]
attribute if I recall correctly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It does indeed so unneeded.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So I assume I can delete this comment then and rustdoc is adding an #[inline]
or should I add it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, I take it back, #[inline]
is _not_currently rendered by rustdoc. Hmmm
https://doc.rust-lang.org/nightly/std/vec/struct.Vec.html#method.new
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It should be though! I open an issue for it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"in place" sounds different than "this function is marked inline" to me; we should find a better wording if it's not, or remove it if it is supposed to communicate that.
@skade What should I add/change to finish this one? |
src/libstd/path.rs
Outdated
fn from(boxed: Box<Path>) -> PathBuf { | ||
boxed.into_path_buf() | ||
} | ||
} | ||
|
||
#[stable(feature = "box_from_path_buf", since = "1.20.0")] | ||
impl From<PathBuf> for Box<Path> { | ||
/// Converts a `PathBuf` into a `Box<Path>`. | ||
/// This conversion does not allocate memory |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think in many cases we'll need to check in with @rust-lang/libs but in obvious cases like this, it's meant to do so and so should be.
src/libstd/path.rs
Outdated
@@ -1419,20 +1423,28 @@ impl<'a, T: ?Sized + AsRef<OsStr>> From<&'a T> for PathBuf { | |||
|
|||
#[stable(feature = "rust1", since = "1.0.0")] | |||
impl From<OsString> for PathBuf { | |||
/// Converts a `OsString` into a `PathBuf`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The format is generally "summary sentence, then other stuff". If you want other stuff, you need a summary. I'm not sure how else you'd summarize this. TL;DR it's fine.
src/libstd/path.rs
Outdated
fn from(path_buf : PathBuf) -> OsString { | ||
path_buf.inner | ||
} | ||
} | ||
|
||
#[stable(feature = "rust1", since = "1.0.0")] | ||
impl From<String> for PathBuf { | ||
/// Converts a `String` into a `PathBuf`. | ||
/// This conversion does not allocate memory |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It should be the same for all of them.
src/libstd/path.rs
Outdated
@@ -1520,6 +1530,9 @@ impl<'a> From<Cow<'a, Path>> for PathBuf { | |||
|
|||
#[stable(feature = "shared_from_slice2", since = "1.24.0")] | |||
impl From<PathBuf> for Arc<Path> { | |||
/// Converts a `PathBuf` into a `Arc<Path>`. | |||
/// This conversion happens in place. | |||
/// This conversion does not allocate memory. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because this one is logically three parts, you'll need two blank lines to separate them out here
src/libstd/path.rs
Outdated
@@ -1529,6 +1542,9 @@ impl From<PathBuf> for Arc<Path> { | |||
|
|||
#[stable(feature = "shared_from_slice2", since = "1.24.0")] | |||
impl<'a> From<&'a Path> for Arc<Path> { | |||
/// Converts a `PathBuf` into a `Arc<Path>`. | |||
/// This conversion happens in place. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"in place" sounds different than "this function is marked inline" to me; we should find a better wording if it's not, or remove it if it is supposed to communicate that.
@gruberb sorry for taking a while here! I'll try to stay on top of this PR better :) |
Ping from triage @steveklabnik! This PR needs your review. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@alexcrichton left a comment that we can't guarantee that these don't allocate, so I think we have to remove all of that
@steveklabnik So it's still valuable according to #51753 (comment). Any ideas on the next steps? |
Ping from triage @steveklabnik. It looks like it's a bit unclear on how this PR should be changed. (Should mention of allocations be removed or just clarified that the behavior may change in the future?) Could you clarify? |
Ping from triage! This PR needs a review, can @steveklabnik or someone else from @rust-lang/docs review this? |
Taking a look at these, i'm concerned with the statements being applied to a few impls. There seems to be a misunderstanding of the underlying mechanisms (and i don't blame you, this pile of implementation details goes all over the place and impressively deep) that has caused the resulting documentation to get out of sync with its implementation. The comments about conversions happening "in place" are misleading, because that's not what's implied by the Let's break down the (current) behavior for the impls in question, because i believe several of them are making incorrect statements:
|
TL;DR: I can confirm that this is only a pointer conversion on all platforms. The intended design is that the encoding of This intent is to some extent already locked in by stable APIs:
|
@bors: r? QuietMisdreavus |
Also, please add examples. |
The job Click to expand the log.
I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
Ping from triage @QuietMisdreavus: It looks like this PR is now ready for your review! |
Ping from triage: @rust-lang/docs -- can anyone drive this PR to completion? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One last nit, then this looks good to go! Thanks for sticking through with this!
Ping @QuietMisdreavus: added the comment slash! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks so much for sticking with this! Sorry that it turned out to be a bigger deal than anticipated; some things turn out to be a little complicated to document! 😅
I think, given the rest of the discussion, that the docs currently in the PR are fine to merge. If you want to come back and add some examples to these docs like @GuillaumeGomez suggested, you can make a new PR once this lands. This PR is old enough that i don't want to hold it up any longer - what's here is good enough to add as-is. @bors r+ rollup |
📌 Commit 450a8a6 has been approved by |
…ibstdpath, r=QuietMisdreavus Document `From` implementations This PR is solves part of rust-lang#51430. It's my first PR, so I might need some guidance from @skade (as already mentioned in the issue). The purpose of the PR is to document the `impl From` inside `path.rs` and answering the questions: - What does it convert? - Does it allocate memory? - How expensive are the allocations? I gave it a first shot, though an experienced rust developer might want to look over it.
…ibstdpath, r=QuietMisdreavus Document `From` implementations This PR is solves part of rust-lang#51430. It's my first PR, so I might need some guidance from @skade (as already mentioned in the issue). The purpose of the PR is to document the `impl From` inside `path.rs` and answering the questions: - What does it convert? - Does it allocate memory? - How expensive are the allocations? I gave it a first shot, though an experienced rust developer might want to look over it.
…ibstdpath, r=QuietMisdreavus Document `From` implementations This PR is solves part of rust-lang#51430. It's my first PR, so I might need some guidance from @skade (as already mentioned in the issue). The purpose of the PR is to document the `impl From` inside `path.rs` and answering the questions: - What does it convert? - Does it allocate memory? - How expensive are the allocations? I gave it a first shot, though an experienced rust developer might want to look over it.
…ibstdpath, r=QuietMisdreavus Document `From` implementations This PR is solves part of rust-lang#51430. It's my first PR, so I might need some guidance from @skade (as already mentioned in the issue). The purpose of the PR is to document the `impl From` inside `path.rs` and answering the questions: - What does it convert? - Does it allocate memory? - How expensive are the allocations? I gave it a first shot, though an experienced rust developer might want to look over it.
Rollup of 15 pull requests Successful merges: - #51753 (Document `From` implementations) - #55563 (Improve no result found sentence in doc search) - #55987 (Add Weak.ptr_eq) - #56119 (Utilize `?` instead of `return None`.) - #56372 (Refer to the second borrow as the "second borrow" in E0501.rs) - #56388 (More MIR borrow check cleanup) - #56424 (Mention raw-ident syntax) - #56452 (Remove redundant clones) - #56456 (Handle existential types in dead code analysis) - #56466 (data_structures: remove tuple_slice) - #56476 (Fix invalid line number match) - #56497 (cleanup: remove static lifetimes from consts in libstd) - #56498 (Fix line numbers display) - #56523 (Added a bare-bones eslint config (removing jslint)) - #56538 (Use inner iterator may_have_side_effect for Cloned) Failed merges: r? @ghost
This PR is solves part of #51430. It's my first PR, so I might need some guidance from @skade (as already mentioned in the issue).
The purpose of the PR is to document the
impl From
insidepath.rs
and answering the questions:I gave it a first shot, though an experienced rust developer might want to look over it.