Windows rs #4635
Windows rs #4635
Conversation
| @@ -277,10 +295,11 @@ impl GlState { | |||
| } | |||
|
|
|||
| let res = unsafe { SetPixelFormat(hdc, format_id, &pfd) }; | |||
| if res == 0 { | |||
| if res.is_err() { | |||
There was a problem hiding this comment.
For something like this, where we are now getting an error as a result, I would prefer something like:
if let Err(err) = res {
anyhow::bail!("SetPixelFormat function failed: {err:#}");
}There was a problem hiding this comment.
or perhaps even just:
unsafe { SetPixelFormat(hdc, format_id, &pfd) }.context("SetPixelFormat")?;(and can we remove the unsafe from here?)
window/src/os/windows/event.rs
Outdated
| @@ -11,29 +9,26 @@ unsafe impl Sync for EventHandle {} | |||
| impl Drop for EventHandle { | |||
| fn drop(&mut self) { | |||
| unsafe { | |||
| CloseHandle(self.0); | |||
| CloseHandle(self.0).unwrap(); | |||
There was a problem hiding this comment.
Semantically speaking, we were ignoring a possible error result before, so we should continue to do so here, especially because panic from within a destructor can be awkward to deal with.
| CloseHandle(self.0).unwrap(); | |
| CloseHandle(self.0).ok(); |
window/src/os/windows/event.rs
Outdated
| Ok(Self(handle)) | ||
| } | ||
|
|
||
| pub fn set_event(&self) { | ||
| unsafe { | ||
| SetEvent(self.0); | ||
| SetEvent(self.0).unwrap(); |
There was a problem hiding this comment.
I try really hard to avoid bare .unwrap(). Suggested alternatives:
- If the error is "totally impossible" and "can never happen", use
.expect("reason this can never happen")so that the code documents why it is impossible - If the error is possible at runtime and we just don't care, consider using
.ok()instead of.unwrap()to discard themust_useattribute - If the error is possible at runtime and something we might need to handle, change the signature of the calling function to some kind of
Resultand allow the caller to decide whether and how to handle it
| @@ -101,110 +100,110 @@ fn build_map() -> HashMap<WPARAM, PhysKeyCode> { | |||
| (0x34, PhysKeyCode::K4), | |||
| (0x36, PhysKeyCode::K6), | |||
| (0x35, PhysKeyCode::K5), | |||
| (VK_OEM_PLUS, PhysKeyCode::Equal), | |||
| (VK_OEM_PLUS.0, PhysKeyCode::Equal), | |||
There was a problem hiding this comment.
Not sure what the answer is, but the .0 here is ugly and it would be great to eliminate it if possible.
window/src/os/windows/wgl.rs
Outdated
| @@ -173,7 +191,7 @@ impl GlState { | |||
| } | |||
|
|
|||
| let extensions = if ext.GetExtensionsStringARB.is_loaded() { | |||
| unsafe { cstr(ext.GetExtensionsStringARB(hdc as *const _)) } | |||
| unsafe { cstr(ext.GetExtensionsStringARB(std::mem::transmute(hdc))) } | |||
There was a problem hiding this comment.
in general, it is best to avoid transmute and take great care with it, because it will let you cast pretty much anything to anything without complaint, and is a great way to mask serious problems. So please find a way to make this work without an inline transmute in here!
There was a problem hiding this comment.
(and same deal for other uses of transmute throughout!)
| }; | ||
| use windows::Win32::UI::Shell::{DragAcceptFiles, DragFinish, DragQueryFileW, HDROP}; | ||
| use windows::Win32::UI::TextServices::HKL; | ||
| use windows::Win32::UI::WindowsAndMessaging::{ |
There was a problem hiding this comment.
This is big enough that it is starting to dominate the file, so I think that a wildcard import would be totally fine.
| use windows::Win32::UI::WindowsAndMessaging::{ | |
| use windows::Win32::UI::WindowsAndMessaging::*; |
|
Just seen the comments @wez and some are addressed in the latest revision anyway, though not all. Some of these were temp stop gaps in proving this would work, I agree about things like the I think the latest status is that the bulk of the heavy work is done (might be a few contentious things like Thanks for reviewing anyway though, especially before it was ready, apologies if that wasn't clear. |
the Win32 keyboard settings.
No description provided.