-
Notifications
You must be signed in to change notification settings - Fork 707
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
Added pointer arrays option #1564
Conversation
Added CLI support and tests for with and without the flag |
btw, I see that it uses |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! This is a good start, some comments below.
tests/headers/array_pointers.h
Outdated
@@ -0,0 +1,4 @@ | |||
|
|||
int test_fn(float a, int arr[20]); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should also add a test with typedefs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The problem with typedefs
is that they are treated as regular arrays for some reason, I'm not sure if it's the right behavior or not. this means that this is kind of already the default thing but without a pointer:
typedef int arr1[20];
typedef int* arr2[20];
typedef int (*arr3)[20];
pub type arr1 = [::std::os::raw::c_int; 20usize];
pub type arr2 = [*mut ::std::os::raw::c_int; 20usize];
pub type arr3 = *mut [::std::os::raw::c_int; 20usize];
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Those typedefs look correct to me. What I meant is that something like: void foo(arr1 a)
generates sane code. It should generate an in pointer without your patch, which is the right thing, but it should generate a: *mut arr1
with your patch, not just a: arr1
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added. tell me if you're happy with the tests.
c_floats are always f32: https://doc.rust-lang.org/std/os/raw/type.c_float.html But sure, it should probably be changed. Probably a different patch though. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! This looks good. I think that a couple tests with typedefs wouldn't hurt though.
Let me know if you don't have cycles to do it and I'll get to adding the tests myself some point this or next week.
Thanks for the patch!
t.to_rust_ty_or_opaque(ctx, &()) | ||
}; | ||
stream.to_ptr(ctx.resolve_type(t).is_const()) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is much more concise than the initial version :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks :)
src/codegen/mod.rs
Outdated
.to_ptr(ctx.resolve_type(t).is_const()) | ||
}, | ||
let stream = if ctx.options().array_pointers_in_arguments { | ||
(*arg_ty).to_rust_ty_or_opaque(ctx, &arg_item) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you need the explicit dereference?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Weird. I did this because it complained but now it works without
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yup! Looks great, thanks!
Could you publish a new version? |
Done :) |
Thanks! :) |
This adds an option to expose
arr[size]
as*mut [T; size]
instead of*mut T
Resolves #1561