Skip to content

Conversation

pnkfelix
Copy link
Contributor

@pnkfelix pnkfelix commented Sep 4, 2013

A dialogue on PR #8909 inspired me to make this change.

r? anyone

(It is possible that std::path itself will soon be replaced with a new implementation that kballard's working on, as mentioned in the dialogue linked above, but this revision is simple enough that I figured I'd offer it up.)

Note that I left dirname as returning ~str, because both of its
implementations work by calling dir_path, which produces a new path,
and thus we cannot borrow the result from &'a self passed to dirname
(because the new path returned by dir_path will not live long enough
to satisfy the lifetime 'a).
In most cases this involved removing a ~str allocations or clones
(yay), or coercing a ~str to a slice.  In a few places, I had to bind
an intermediate Path (e.g. path.pop() return values), so that it would
live long enough to support the borrowed &str.

And in a few places, where the code was actively using the property
that the old API returned ~str's, I had to put in to_owned() or
clone(); but in those cases, we're trading an allocation within the
path.rs code for one in the client code, so they neutralize each
other.
@lilyball
Copy link
Contributor

lilyball commented Sep 4, 2013

Looks trivially correct to me. My current work on rewriting path is on my path-rewrite branch, but it's not ready yet, so @pnkfelix's work may as well be used for now.

bors added a commit that referenced this pull request Sep 5, 2013
…py, r=huonw

A [dialogue](#8909 (diff)) on PR #8909 inspired me to make this change.

r? anyone

(It is possible that `std::path` itself will soon be replaced with a new implementation that kballard's working on, as mentioned in the dialogue linked above, but this revision is simple enough that I figured I'd offer it up.)
@bors bors closed this Sep 5, 2013
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.

4 participants