-
Notifications
You must be signed in to change notification settings - Fork 465
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Improved Win32 array transformation #1570
Conversation
I would love to be able to use these method signature to be able to avoid the possibility of runtime failures, but I'm not sure if this is good idea because this makes it impossible to call the APIs with dynamic length slices - you could only call the APIs with compile-time fixed length arrays. If the compile-time length restriction is a problem, I think the API wrapper may be forced to add run-time checks. I'm not sure how the runtime checks would emit failures - perhaps panic, as failing them would be a programmer error? Perhaps 2 different signatures could be provided to allow for trading off compile-time checks for runtime flexibility? |
@samlh I wondered about that, but you can still use a slice if you so desire, you just have to use a run time check to confirm that it has the right length. For example, the fn wrapper(buffer: &mut [u8]) {
unsafe {
GetTempFileNameA(".", "test", 0x7b, buffer.try_into().expect("slice length not 260"));
}
} |
Right, that works if you know at compile time what length you need to cast to. However, for a function like ID3D12CommandQueue::UpdateTileMappings, you might want to call it with 2 regions on one frame, and 3 regions on another frame, so the caller doesn't have a single compile-time length to cast to. To put it another way |
Got it - that's a good example. |
One approach to this problem is to make a new type to represent the invariant, with multiple constructors (untested): struct SameLenSlices2<T1,T2>(t1: &[T1], t2: &[T2]);
impl<T,U> TryInto<SameLenSlices2<T,U> for (&[T],&[U]) { ... }
impl<const LEN: usize,T,U> Into<SameLenSlices2<T,U> for (&[T;LEN],&[U;LEN]) { ... }
// Repeat for SameLenSlices3, etc
fn UpdateTileMappings(regions: SameLenSlices2<D3D12_TILED_RESOURCE_COORDINATE, D3D12_TILE_REGION_SIZE>, ...) Either way, callers would need to do I'm throwing this out as an idea, but I do think it is likely overkill... |
For what its worth, just tested this out and it is working great for the specific use case in #1567 but what @samlh brought up seems like a pretty big dealbreaker. I wonder if it would be appropriate to just not length check the slices and allowing the length parameter to be passed as part of the call signature while still going through with the Not familiar enough with bindgen to know if such a transform will require much more extra code at runtime. If the caller is not careful there is obvious unsoundness here but as its As a related aside, perhaps that would allow similar transforms for functions that take pub unsafe fn RSGetScissorRects(&self, pnumrects: *mut u32, prects: *mut RECT); and pub unsafe fn CSGetShader(&self, ppcomputeshader: *mut Option<ID3D11ComputeShader>, ppclassinstances: *mut Option<ID3D11ClassInstance>, pnumclassinstances: *mut u32) respectively, dereferencing the array length parameter as necessary as the disambiguator of the input slice lengths. It would still be dangerous (arguably no more than it is dangerous to use in C++), but the ergonomics would be much better than the |
Thanks for the feedback! I'm just taking a few days off, but I'll likely scale this back to avoid such ambiguities for now. Separately I'm hoping to reduce the number of methods that must be |
Ended up scaling back the changes to only include the parsing improvements and very limited code gen changes to improve correctness. It avoids all parameters that share a length and have any optional parameters. |
Improves win32 array parsing and fixes a few minor correctness issues with shared array lengths.