fix: std::sys::fs use_with_native_path for read_dir for windows#148045
fix: std::sys::fs use_with_native_path for read_dir for windows#148045WrldEngine wants to merge 8 commits intorust-lang:mainfrom
Conversation
rustbot
added
S-waiting-on-review
|
rustbot has assigned @Mark-Simulacrum. Use |
This comment was marked as outdated.
This comment was marked as outdated.
rustbot
added
has-merge-commits
WrldEngine
force-pushed
the
sys_windows
branch
from
85f3ca8 to
0527ec1
Compare
rustbot
removed
has-merge-commits
|
@rustbot label +O-windows |
|
cc @ChrisDenton |
|
|
But it is beginning of fixing the purpose using this function on all platforms. I covered it on windows, expensive no-op it is the function 'use_with_native_path' at all |
|
The purpose of the fixme is to clearly show that there's an issue to be fixed. This PR doesn't fix the issue, it merely hides the issue one level deeper and removes the fixme note. The performance impact of this is why it's a fixme instead of being implemented that way to start with. |
I didnt remove fixme comment Also i see some moments like: pub fn copy(from: &Path, to: &Path) -> io::Result<u64> {
// FIXME: use with_native_path on all platforms
#[cfg(not(windows))]
return imp::copy(from, to);
#[cfg(windows)]
with_native_path(from, &|from| with_native_path(to, &|to| imp::copy(from, to)))
}is it also expensive no-op? |
|
Yes, if |
|
r? @ChrisDenton |
|
Requested reviewer is already assigned to this pull request. Please choose another assignee. |
| // We push a `*` to the end of the path which cause the empty path to be | ||
| // treated as the current directory. So, for consistency with other platforms, | ||
| // we explicitly error on the empty path. | ||
| if p.as_os_str().is_empty() { | ||
| if p.is_empty() { | ||
| // Return an error code consistent with other ways of opening files. | ||
| // E.g. fs::metadata or File::open. | ||
| return Err(io::Error::from_raw_os_error(c::ERROR_PATH_NOT_FOUND as i32)); | ||
| } |
There was a problem hiding this comment.
This comment and check should stay the first thing this function does; there is no sense in creating an empty Vec, but then fail here...
| pub const fn to_wchars_with_null_unchecked(&self) -> &[u16] { | ||
| &self.0 | ||
| } | ||
|
|
5 participants
Fix the read_dir function in windows for using with native_path
r? @ChrisDenton