Prefer &T to *const T for Win32 input parameters
#1939
Conversation
There was a problem hiding this comment.
I think this is fine, but I am a bit worried about if this ends up making some operations unsafe. &T and &mut T are just specific types of pointers so it should be fine to accept them as parameters and convert them to their respective raw pointer types at the FFI boundary. However, this may force the user to convert raw pointer types into Rust reference types which can be tricky to do in safe way.
For example, if I call an API which returns a raw pointer to me, and then I need to call another API that uses that return value as an argument, this change may force me to convert that raw API into a Rust reference. But this might be difficult to do in a way that is safe. &T and &mut T come with many different guarantees that must be upheld for there not to be UB.
For example, &mut T must:
- not be null
- be exclusive (i.e., not aliased at all).
- the referent must be valid for the entire time that the reference is alive for
- etc.
I didn't see any changes to the examples that looked suspect, but we should keep an eye out for this.
| @@ -23,7 +23,8 @@ fn test() -> Result<()> { | |||
|
|
|||
| let mut random = GUID::zeroed(); | |||
|
|
|||
| BCryptGenRandom(provider, &mut random as *mut _ as _, core::mem::size_of::<GUID>() as _, 0)?; | |||
| // TODO: workaround for https://github.com/microsoft/windows-rs/issues/1938 | |||
| BCryptGenRandom(provider, std::mem::transmute(&mut random), core::mem::size_of::<GUID>() as _, 0)?; | |||
There was a problem hiding this comment.
What is &mut random being transmuted into?
There was a problem hiding this comment.
pub unsafe fn BCryptGenRandom<'a, P0>(halgorithm: P0, pbbuffer: &mut u8, cbbuffer: u32, dwflags: u32) -> ::windows::core::Result<()>But #1938 should turn that into a slice - still require some kind of explicit cast tho.
| @@ -45,7 +45,7 @@ fn test() -> Result<()> { | |||
| assert!(b.GetCount()? == 123); | |||
|
|
|||
| let c: IPropertyStoreCapabilities = b.cast()?; | |||
| c.IsPropertyWritable(std::ptr::null())?; | |||
| c.IsPropertyWritable(&PROPERTYKEY::default())?; | |||
There was a problem hiding this comment.
This seems different than it was before. Why isn't None being passed?
There was a problem hiding this comment.
This parameter is not optional.
|
Yep, I looked for issues and didn't come across any (other than |
|
This pr changed IWTSVirtualChannelManager.CreateListener to take a &u8 instead of a *const u8. I was able to get a *const u8 by simply calling as_ptr() on a null terminated string, but obviously that does no longer work. Would it be possible to document cases like this, or should they be excluded as well? |
|
It may be a regression - can you open a new issue if you believe that may be the case? Otherwise I may lose track of it. |
|
Created microsoft/win32metadata#1185 |
A relatively small change to code gen has a big impact on usability. Basically, Win32 pointer parameters are turned into references. Of those, optional parameters are wrapped in
Option. I have avoided applying this to void pointers for the time being as they are a little trickier to deal with. The effect in usability is very compelling. For example:CreateEventAgoes from this:To this:
Direct2D's
EndDrawmethod goes from this:To this:
Non-optional pointer parameter are now immutable references, which doesn't really affect calling patterns but makes it a lot less confusing when reading the docs.
Mutable pointers are now mutable references, wrapped in
Optionas needed.Unfortunately, it also highlighted a few more metadata bugs:
microsoft/win32metadata#1005
microsoft/win32metadata#1006
Overall, a very nice improvement for working with Win32 API calls.
Fixes #1441
Fixes #816