Skip to content

Rename std::path to std::old_path; introduce new std::path#21759

Merged
bors merged 2 commits intorust-lang:masterfrom
aturon:new-path
Feb 4, 2015
Merged

Rename std::path to std::old_path; introduce new std::path#21759
bors merged 2 commits intorust-lang:masterfrom
aturon:new-path

Conversation

@aturon
Copy link
Contributor

@aturon aturon commented Jan 29, 2015

This PR implements path reform, and motivation and details for the change can be found there.

For convenience, the old path API is being kept as old_path for the time being. Updating after this PR is just a matter of changing imports to old_path (which is likely not needed, since the prelude entries still export the old path API).

This initial PR does not include additional normalization or platform-specific path extensions. These will be done in follow up commits or PRs.

[breaking-change]

Closes #20034
Closes #12056
Closes #11594
Closes #14028
Closes #14049
Closes #10035

@aturon
Copy link
Contributor Author

aturon commented Jan 29, 2015

r? @alexcrichton

@rust-highfive rust-highfive assigned alexcrichton and unassigned huonw Jan 29, 2015
@rust-highfive
Copy link
Contributor

r? @huonw

(rust_highfive has picked a reviewer for you, use r? to override)

Copy link
Contributor

Choose a reason for hiding this comment

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

If we have a path C:foo.txt this is a relative path, not an absolute path, so it doesn't make sense to replace self outright.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Consider pushing C:foo.txt onto D:\temp. What would you want to happen?

The existing path API appends the path only for drive prefixes where the volumes match. I wrote some code to do the same initially, but ultimately felt that it was inconsistent behavior, rather than always treating pushing C:foo.txt as a drive-CWD-relative path. But I could be convinced otherwise! The tradeoffs aren't completely clear to me.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Update: based on an IRC chat with @retep998, it's unclear that drive-CWD is available in winapi (though it is within cmd). Also, PathCchCombineEx (from https://msdn.microsoft.com/en-us/library/windows/desktop/hh707086%28v=vs.85%29.aspx) behaves identically to the API proposed here.

I'm somewhat uncomfortable with pushing C:foo.txt onto D:\temp resulting in C:\temp\foo.txt, but I really don't know in what contexts this situation is likely to come up (and what you'd want it to do for those).

Copy link
Contributor

Choose a reason for hiding this comment

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

As it turns out, Windows does have drive specific current directories. Unfortunately I cannot find a way to directly get the CWD of a specific drive. Also the CWD is a very racy thing that is shared across threads and may change values on you between calls.
Since methods in winapi treat C:foo.txt as an absolute path when joining, we should replicate that behavior.
Any method that actually interacts with the filesystem and is given a C:foo.txt will treat it as a path relative to the CWD on that drive. Therefore when normalizing the path based on the filesystem we should probably use GetFullPathNameW to get the absolute path. The only other method I can think of involves changing the CWD to the drive of that path, and then getting the CWD, which is undesirable because it changes the CWD for other threads.

Copy link
Member

Choose a reason for hiding this comment

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

Not sure if I'll see something down below, but are paths like C:\foo and c:\foo equivalent? If so, does this derived implementation handle that?

Copy link
Member

Choose a reason for hiding this comment

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

Ah it looks like PartialEq is on Component below, but also not case-insensitive, just confirming that is desired.

Copy link
Contributor

Choose a reason for hiding this comment

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

Unfortunately case sensitivity is very hard for us to do right, because the case sensitivity tables are stored in the partition itself. This means that we cannot determine whether two paths are equal if they differ in case unless we ask the filesystem to resolve the two paths for us.
However, we can at least assume that the drive letter is always case insensitive so C:\foo and c:\foo should always be considered equal.

Copy link
Member

Choose a reason for hiding this comment

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

Ah yeah I would only consider the drive letter to be considered for case insensitivity, not the actual components of the path itself. Thanks for clarifying!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment