-
Notifications
You must be signed in to change notification settings - Fork 13.3k
Refactor std::path to use default methods #8909
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
Conversation
@@ -57,54 +57,115 @@ pub fn PosixPath(s: &str) -> PosixPath { | |||
GenericPath::from_str(s) | |||
} | |||
|
|||
pub trait GenericPath { | |||
/// Converts a string to a Path | |||
pub trait GenericPath : Clone + Eq + ToStr { |
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 looks like this API is pretty old, and more recent apis have since been attempting to remove as many allocations as possible. It appears that this API itself is performing a lot of allocations all over the place when most of them aren't necessary. I'm not sure if you'd want to look into this in this pull request or possibly leave it to later, though. If you're modifying Path
already it seems like a good time, but if you're busy, then no worries!
Regardless, this inheritance is fine (especially if the traits are usable in the default implementations of the methods). If you can remove them, however, feel free to. I'd love to drop the Clone
bound, but that's coupled with changing these functions to perform fewer allocations anyway.
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.
Re: "especially if the traits are usable in the default implementations of the methods" Yes, WindowsPath
and PosixPath
were already deriving from Clone
, and now that some of the code from those has been moved up into default methods, I had to do this in order to use the traits in the default methods.
I'll see what I can do about having this code perform fewer allocations, but I probably will need some advice and examples.
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 having a go at revising the Path API to have fewer allocations built in.
But its a bigger job than alexcrichton might have realized, because some clients (e.g. rustpkg) are taking advantage of the fact that the accessors return owned strings. Its not hard to fix, but just a bigger job and probably worthy of a separate ticket. It does not have to block landing lkuper's work here, if someone else wants to double check the concerns lkuper raised in the ticket description.
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 believe @kballard is also experimenting with rewriting std::path
.
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 am. I'm pretty far along. I told @lkuper in IRC that refactoring to use default methods is fine and won't interfere with my work, but doing any more work on path is probably not a good idea as I'm replacing it entirely.
I'm currently on vacation but I will be resuming my work on the new path next week.
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.
Hmm. I think I just finished the task of revising the current Path API to have fewer allocations built in. I'm going to go ahead and file the PR for it; if we end up switching to @kballard's API later, fine, but I figure I'll be happier if we fix this now.
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.
(PR filed as #8978, just to tie off that task.)
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.
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.
Rebased and updated the patch.
…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.)
fn filename<'a>(&'a self) -> Option<&'a str> { | ||
match self.components().len() { | ||
0 => None, | ||
n => Some(self.components()[n - 1].clone()), |
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.
Does this actually compile? I'd be a little surprised if it did. Regardless I don't think the clone is necessary.
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 what part of it did you think looked suspcious? The use of 0
in a match pattern?
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 clone should create an ~
pointer which does not have the same lifetime as self
, yet you're returning it as the same lifetime.
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.
Ah! Scary!
Okay, I just tried out something similar in a little example, and the type checker yelled at me (expected &'a but found ~
). So you are right: this code shouldn't compile, and probably does not.
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.
@pnkfelix @alexcrichton Yes, you were right, this doesn't compile, and shouldn't. I messed up the merge, then was sloppy and didn't wait for LLVM to build. I just updated the patch to a version that actually compiles and passes make check-stage1-std
.
lgtm with removal of allocations |
Updated patch to incorporate #8978. r? anyone |
…chton (cc: #3227) Parts I'm unsure about and would like a reviewer to look at are: * `pub trait GenericPath : Clone + Eq + ToStr` -- is this the done thing? I've never done trait inheritance before, let alone from multiple traits, but it seemed to be necessary to be able to call all the methods we have to be able to call on `self`. * changing the argument of `components` from `self` to `&self`, and having it return `self.components.clone()` instead of `self.components`; this was necessary to avoid move errors, but I'm not sure if it's the right thing. (The default methods impls now all have to call `self.components()` instead of just referencing the field `self.components`.)
…chton (cc: #3227) Parts I'm unsure about and would like a reviewer to look at are: * `pub trait GenericPath : Clone + Eq + ToStr` -- is this the done thing? I've never done trait inheritance before, let alone from multiple traits, but it seemed to be necessary to be able to call all the methods we have to be able to call on `self`. * changing the argument of `components` from `self` to `&self`, and having it return `self.components.clone()` instead of `self.components`; this was necessary to avoid move errors, but I'm not sure if it's the right thing. (The default methods impls now all have to call `self.components()` instead of just referencing the field `self.components`.)
…py, r=huonw A [dialogue](rust-lang/rust#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.)
(cc: #3227)
Parts I'm unsure about and would like a reviewer to look at are:
pub trait GenericPath : Clone + Eq + ToStr
-- is this the done thing? I've never done trait inheritance before, let alone from multiple traits, but it seemed to be necessary to be able to call all the methods we have to be able to call onself
.components
fromself
to&self
, and having it returnself.components.clone()
instead ofself.components
; this was necessary to avoid move errors, but I'm not sure if it's the right thing. (The default methods impls now all have to callself.components()
instead of just referencing the fieldself.components
.)