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

Document From implementations #51753

Merged
merged 7 commits into from
Dec 6, 2018
Merged

Document From implementations #51753

merged 7 commits into from
Dec 6, 2018

Conversation

gruberb
Copy link
Contributor

@gruberb gruberb commented Jun 24, 2018

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 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.

@rust-highfive

This comment has been minimized.

@pietroalbini
Copy link
Member

Restarted the Travis CI build, the failure was caused by the crates.io outage.

@pietroalbini pietroalbini added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jun 24, 2018
@pietroalbini
Copy link
Member

Assigning someone from the libs team. r? @sfackler

Copy link
Contributor

@skade skade left a 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 }
Copy link
Contributor

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!

Copy link
Contributor Author

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.

@@ -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`.
Copy link
Contributor

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.

Copy link
Member

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.

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.
Copy link
Contributor

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!

https://godbolt.org/g/Aoe8Q1

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
Copy link
Contributor

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.

Copy link
Member

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.

/// 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.
Copy link
Contributor

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.

Copy link
Contributor Author

@gruberb gruberb Jun 25, 2018

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?

Copy link
Contributor

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.

/// 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.
Copy link
Contributor

Choose a reason for hiding this comment

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

Same here.

/// 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.
Copy link
Contributor

Choose a reason for hiding this comment

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

Same here.

@stokhos
Copy link

stokhos commented Jun 29, 2018

Ping from triage @sfackler , will you have time to review this?

@sfackler
Copy link
Member

I'll defer to the docs team.

r? @steveklabnik

@stokhos stokhos added the A-docs Area: documentation for any part of the project, including the compiler, standard library, and tools label Jul 6, 2018
@stokhos
Copy link

stokhos commented Jul 6, 2018

Ping from triage @steveklabnik @rust-lang/docs guys, this PR needs your review.

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
Copy link
Member

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.

Copy link
Member

@GuillaumeGomez GuillaumeGomez Jul 8, 2018

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.

Copy link
Member

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.

Copy link
Member

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.

Copy link
Contributor Author

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?

Copy link
Member

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

@@ -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.
Copy link
Member

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.

Copy link
Member

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.

Copy link
Contributor Author

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?

Copy link
Member

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


screen shot 2018-07-09 at 9 17 29 am


screen shot 2018-07-09 at 9 17 35 am


https://doc.rust-lang.org/nightly/std/vec/struct.Vec.html#method.new

Copy link
Member

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.

Copy link
Member

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
Copy link
Contributor Author

gruberb commented Jul 20, 2018

@skade What should I add/change to finish this one?

src/libstd/path.rs Outdated Show resolved Hide resolved
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
Copy link
Member

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.

@@ -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`.
Copy link
Member

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.

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
Copy link
Member

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.

@@ -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.
Copy link
Member

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

@@ -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.
Copy link
Member

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.

@steveklabnik
Copy link
Member

@gruberb sorry for taking a while here! I'll try to stay on top of this PR better :)

@pietroalbini
Copy link
Member

Ping from triage @steveklabnik! This PR needs your review.

Copy link
Member

@steveklabnik steveklabnik left a 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

@gruberb
Copy link
Contributor Author

gruberb commented Aug 3, 2018

@steveklabnik So it's still valuable according to #51753 (comment). Any ideas on the next steps?

@TimNN
Copy link
Contributor

TimNN commented Aug 7, 2018

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?

@pietroalbini pietroalbini added S-waiting-on-team Status: Awaiting decision from the relevant subteam (see the T-<team> label). and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Aug 27, 2018
@pietroalbini
Copy link
Member

Ping from triage! This PR needs a review, can @steveklabnik or someone else from @rust-lang/docs review this?

@QuietMisdreavus
Copy link
Member

QuietMisdreavus commented Aug 27, 2018

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 #[inline] attribute. If i'm looking for the allocation characteristics of a conversion, if i see "in place" i assume that there is neither an allocation nor a data copy.

Let's break down the (current) behavior for the impls in question, because i believe several of them are making incorrect statements:

  • impl From<OsString> for PathBuf, impl From<PathBuf> for OsString, impl From<Box<Path>> for PathBuf
    • These are effectively pointer conversions, so i would say these happen "in place", without allocating memory:

      This conversion does not allocate or copy memory.

  • impl From<PathBuf> for Box<Path>
    • The memory characteristics of this one depend on how shrink_to_fit is handled by the allocator. Based on discussion in this thread, this shouldn't allocate or copy most of the time, but i'm not comfortable making an absolute statement on it. This one can probably be rephrased to something like:

      This conversion currently should not allocate memory, but this behavior is not guaranteed on all platforms or in all future versions.

  • impl From<String> for PathBuf
    • This one is sketchy, as it really depends on how the String -> OsString conversion goes. On Unix and Windows, these look like pointer conversions (take the Vec<u8> out of the String and put it into an OsString based on the UTF-8 guarantees of String), but does this apply to all platforms? I'm not comfortable making a blanket statement here. Here's my idea of a decent statement for this one:

      On Unix and Windows, this conversion currently should not allocate memory, but this behavior is not guaranteed on all platforms or in all future versions.

      (Am i reading the Windows implementation correctly? The use of the WTF-8 buffer and the direct conversion from a Vec<u8> seems to contradict the statement in OsString's documentation.)

      Based on @SimonSapin's confirmation below, and a rereading of the OsString docs, i'm comfortable using this phrasing for this conversion:

      This conversion does not allocate or copy memory.

  • impl From<PathBuf> for Arc<Path>, impl<'a> From<&'a Path> for Arc<Path>, impl From<PathBuf> for Rc<Path> (and the impl<'a> From<&'a Path> for Rc<Path> that you missed right below)
    • These all definitely allocate memory and copy data into the new buffer. The Arc/Rc backing buffers need to hold the item's reference count(s), so a new buffer needs to be created each time. This may be an implementation detail that we may or may not actually want to expose, but to say that these conversions don't allocate memory is incorrect. They could be rephrased to something like:

      Converts a (source type) into a (target type) by copying the Path data into a new (Arc/Rc) buffer.

@SimonSapin
Copy link
Contributor

impl From<String> for PathBuf
On Unix and Windows, these look like pointer conversions […], but does this apply to all platforms?

TL;DR: I can confirm that this is only a pointer conversion on all platforms.


The intended design is that the encoding of OsString is always a superset of UTF-8 (which is String’s encoding). Specifically, on currently-supported platforms, either arbitrary bytes or WTF-8. In other words, the set of values of OsString is a superset of the set of values of String, with common values having the same memory representation.

This intent is to some extent already locked in by stable APIs:

  • impl From<String> for OsString. This conversion being infallible (not returning a Result) implies that all String values can be represented somehow as OsString.

  • OsString::to_str(&self) -> Option<&str> (through Deref<Target = OsStr>). The signature returning a borrow implies that at least some values have a memory representation compatible with UTF-8. I think the documentation requires that all values that can, do.

@steveklabnik
Copy link
Member

steveklabnik commented Sep 5, 2018

@bors: r? QuietMisdreavus

@GuillaumeGomez
Copy link
Member

Also, please add examples.

@Mark-Simulacrum Mark-Simulacrum added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-team Status: Awaiting decision from the relevant subteam (see the T-<team> label). labels Nov 15, 2018
@rust-highfive
Copy link
Collaborator

The job x86_64-gnu-llvm-5.0 of your PR failed on Travis (raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem.

Click to expand the log.
travis_time:end:101a4afc:start=1542804044339965845,finish=1542804046879266452,duration=2539300607
$ git checkout -qf FETCH_HEAD
travis_fold:end:git.checkout

Encrypted environment variables have been removed for security reasons.
See https://docs.travis-ci.com/user/pull-requests/#Pull-Requests-and-Security-Restrictions
$ export SCCACHE_BUCKET=rust-lang-ci-sccache2
$ export SCCACHE_REGION=us-west-1
Setting environment variables from .travis.yml
$ export IMAGE=x86_64-gnu-llvm-5.0
---
[00:04:06] tidy error: /checkout/src/libstd/path.rs:1412: trailing whitespace
[00:04:07] some tidy checks failed
[00:04:07] 
[00:04:07] 
[00:04:07] command did not execute successfully: "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0-tools-bin/tidy" "/checkout/src" "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0/bin/cargo" "--no-vendor" "--quiet"
[00:04:07] 
[00:04:07] 
[00:04:07] failed to run: /checkout/obj/build/bootstrap/debug/bootstrap test src/tools/tidy
[00:04:07] Build completed unsuccessfully in 0:00:58
[00:04:07] Build completed unsuccessfully in 0:00:58
[00:04:07] make: *** [tidy] Error 1
[00:04:07] Makefile:79: recipe for target 'tidy' failed
The command "stamp sh -x -c "$RUN_SCRIPT"" exited with 2.
travis_time:start:2f63ee00
$ date && (curl -fs --head https://google.com | grep ^Date: | sed 's/Date: //g' || true)
Wed Nov 21 12:45:03 UTC 2018
---
travis_time:end:08074933:start=1542804304403494753,finish=1542804304408999905,duration=5505152
travis_fold:end:after_failure.3
travis_fold:start:after_failure.4
travis_time:start:02834ec8
$ ln -s . checkout && for CORE in obj/cores/core.*; do EXE=$(echo $CORE | sed 's|obj/cores/core\.[0-9]*\.!checkout!\(.*\)|\1|;y|!|/|'); if [ -f "$EXE" ]; then printf travis_fold":start:crashlog\n\033[31;1m%s\033[0m\n" "$CORE"; gdb --batch -q -c "$CORE" "$EXE" -iex 'set auto-load off' -iex 'dir src/' -iex 'set sysroot .' -ex bt -ex q; echo travis_fold":"end:crashlog; fi; done || true
travis_fold:end:after_failure.4
travis_fold:start:after_failure.5
travis_time:start:12807180
travis_time:start:12807180
$ cat ./obj/build/x86_64-unknown-linux-gnu/native/asan/build/lib/asan/clang_rt.asan-dynamic-i386.vers || true
cat: ./obj/build/x86_64-unknown-linux-gnu/native/asan/build/lib/asan/clang_rt.asan-dynamic-i386.vers: No such file or directory
travis_fold:end:after_failure.5
travis_fold:start:after_failure.6
travis_time:start:32bf1824
$ dmesg | grep -i kill

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 @TimNN. (Feature Requests)

@TimNN TimNN 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 Nov 27, 2018
@TimNN
Copy link
Contributor

TimNN commented Nov 27, 2018

Ping from triage @QuietMisdreavus: It looks like this PR is now ready for your review!

@Centril Centril added S-waiting-on-team Status: Awaiting decision from the relevant subteam (see the T-<team> label). and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Dec 1, 2018
@Centril
Copy link
Contributor

Centril commented Dec 1, 2018

Ping from triage: @rust-lang/docs -- can anyone drive this PR to completion?

Copy link
Member

@QuietMisdreavus QuietMisdreavus left a 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!

src/libstd/path.rs Outdated Show resolved Hide resolved
@gruberb
Copy link
Contributor Author

gruberb commented Dec 4, 2018

Ping @QuietMisdreavus: added the comment slash!

Copy link
Member

@QuietMisdreavus QuietMisdreavus left a 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! 😅

@QuietMisdreavus
Copy link
Member

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

@bors
Copy link
Contributor

bors commented Dec 4, 2018

📌 Commit 450a8a6 has been approved by QuietMisdreavus

@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-team Status: Awaiting decision from the relevant subteam (see the T-<team> label). labels Dec 4, 2018
GuillaumeGomez added a commit to GuillaumeGomez/rust that referenced this pull request Dec 4, 2018
…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.
GuillaumeGomez added a commit to GuillaumeGomez/rust that referenced this pull request Dec 5, 2018
…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.
pietroalbini added a commit to pietroalbini/rust that referenced this pull request Dec 5, 2018
…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.
pietroalbini added a commit to pietroalbini/rust that referenced this pull request Dec 5, 2018
…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.
bors added a commit that referenced this pull request Dec 5, 2018
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
@bors bors merged commit 450a8a6 into rust-lang:master Dec 6, 2018
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-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.