Replace which with a manual implementation using SearchPathW#96
Replace which with a manual implementation using SearchPathW#96zadjii-msft wants to merge 4 commits intomainfrom
Conversation
| fn get_path_extensions() -> Vec<String> { | ||
| env::var("PATHEXT") | ||
| .unwrap_or_else(|_| { | ||
| // If PATHEXT isn't set, use the default | ||
| ".COM;.EXE;.BAT;.CMD;.VBS;.VBE;.JS;.JSE;.WSF;.WSH;.MSC".to_string() | ||
| }) | ||
| .split(';') | ||
| .map(|ext| ext.to_string()) | ||
| .collect() | ||
| } |
There was a problem hiding this comment.
If this was inside search_path_with_extensions it wouldn't need to collect into a vector first, nor allocate each string chunk.
Like this:
let pathext = env::var("PATHEXT");
let extensions = pathext
.as_deref()
// If PATHEXT isn't set, use the default
.unwrap_or(".COM;.EXE;.BAT;.CMD;.VBS;.VBE;.JS;.JSE;.WSF;.WSH;.MSC")
.split(';');There was a problem hiding this comment.
Thinking about it some more, I think it'd be preferable if we still did this, but it's not something I would block over.
| // the path, including the terminating null character. | ||
| // | ||
| // Includes some padding just in case and because it's cheap. | ||
| buffer.resize((len + 64) as usize, 0); |
There was a problem hiding this comment.
Wait, doesn't this mean that if it fails to fit in the buffer we will move on to the next extension after growing the buffer? That doesn't seem correct.
There was a problem hiding this comment.
Hah. I didn't even catch this! My initial suggestion had a loop, so this looked quite similar. 😅 But it needs a loop in a loop.
| let extension_hstring = HSTRING::from(extension); | ||
| // SearchPathW will ignore the extension if the filename already has one. | ||
| let len: u32 = | ||
| unsafe { SearchPathW(None, &filename, &extension_hstring, Some(&mut buffer), None) }; |
There was a problem hiding this comment.
I am fairly certain that SearchPathW doesn't take an HSTRING o_O
Is this a weird rust-win32ism?
There was a problem hiding this comment.
SearchPathW expects a 16-bit wide string but you're starting with a UTF-8 string. HSTRING provides a 16-bite wide string and comprehensive conversion from various Rust Standard Library types, so it's very convenient in cases like this.
There was a problem hiding this comment.
Unlike BSTR, HSTRING is not compatible when passed as a LPWSTR. It does not point directly to the character data. Why is passing it like a LPWSTR to an API that expects LPWSTR OK, in contravention of all Windows API design to-date?
There was a problem hiding this comment.
(I am happy to accept “there are magic conversions in the rust Windows crate,” but that is exactly what I mean when I ask if it is a weirdness. I also want somebody to say it on-record.)
There was a problem hiding this comment.
I assume by LPWSTR you really mean PCWSTR. 😉
In which case, HSTRING is actually a far better choice than BSTR. While BSTR is literally the pointer, that pointer is far more dangerous in general and BSTR semantics are far more nebulous than HSTRING. An HSTRING is perfectly compatible with PCWSTR and even has an API for that - WindowsGetStringRawBuffer - but I implemented it inline for both C++/WinRT and Rust and there is thus no performance penalty for pointer access. HSTRING is generally more efficient, can avoid heap allocations, and while windows-strings does provide all kinds of conveniences for both HSTRING and BSTR, the former is preferred.
microsoft-github-policy-service
bot
added
the
Needs-Author-Feedback
| // not including the terminating null character. | ||
| if len < buffer.len() as u32 { | ||
| buffer.truncate(len as usize); | ||
| return Ok(String::from_utf16(&buffer)?); |
There was a problem hiding this comment.
FWIW you could also consider returning an OsString or even better Path here, as that would be consistent with other parts of Rust and this app. It would also allow you to revert the change to absolute_path below.
This comment was marked as outdated.
This comment was marked as outdated.
DHowett
changed the title
microsoft-github-policy-service
bot
added
the
No-Recent-Activity
zadjii-msft
removed
No-Recent-Activity
|
(sshhh bot, I was on vacation) |
4 participants
This pull request removes our dependency on
which, which Kenny so helpfully pointed out we don't need: https://gist.github.com/kennykerr/a4375597c7507182570576cf9e7b6ae5