yakuza-freecam icon indicating copy to clipboard operation
yakuza-freecam copied to clipboard

Shrink unsafe blocks

Open cactter opened this issue 2 years ago • 2 comments

In this function you use the unsafe keyword for some safe expressions.

We need to mark unsafe operations more precisely using unsafe keyword. Keeping unsafe blocks small can bring many benefits. For example, when mistakes happen, we can locate any errors related to memory safety within an unsafe block. This is the balance between Safe and Unsafe Rust. The separation is designed to make using Safe Rust as ergonomic as possible, but requires extra effort and care when writing Unsafe Rust.

Hope this PR can help you. Best regards. References https://doc.rust-lang.org/nomicon/safe-unsafe-meaning.html https://doc.rust-lang.org/book/ch19-01-unsafe-rust.html

cactter avatar Jul 22 '22 02:07 cactter

Hi!

I really appreciate the initiative, and you're right, unsafe blocks can be (and should be!) minimized to be able to enjoy rust's guarantees.

I recognize most of this code is very hacky, so any effort of cleaning is appreciated.

If you're still interested, I'd suggest to change the focus of this PR a bit. Particularly, as you saw, most of the time you were shrinking sections of unsafe where the function was GetAsyncKeyState.

According to the docs (https://docs.microsoft.com/en-us/windows/win32/api/winuser/nf-winuser-getasynckeystate) there's no remark when this function could cause undefined behavior, so it may be worth to create a "safe" wrapper, and I'd prefer that path instead of just shrinking the unsafe blocks.

Let me know if you'd be up to trying that.

etra0 avatar Aug 12 '22 03:08 etra0

Hi!

I really appreciate the initiative, and you're right, unsafe blocks can be (and should be!) minimized to be able to enjoy rust's guarantees.

I recognize most of this code is very hacky, so any effort of cleaning is appreciated.

If you're still interested, I'd suggest to change the focus of this PR a bit. Particularly, as you saw, most of the time you were shrinking sections of unsafe where the function was GetAsyncKeyState.

According to the docs (https://docs.microsoft.com/en-us/windows/win32/api/winuser/nf-winuser-getasynckeystate) there's no remark when this function could cause undefined behavior, so it may be worth to create a "safe" wrapper, and I'd prefer that path instead of just shrinking the unsafe blocks.

Let me know if you'd be up to trying that.

Thanks for the suggestion, it's a great idea.

cactter avatar Sep 10 '22 02:09 cactter