-
Notifications
You must be signed in to change notification settings - Fork 12
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
fix(ffi/ada_free_owned_string): c parameter is passed by value #65
Conversation
While working on #63, I noticed that my Drop wasn't correctly working because the Rust Implementation was assuming that the C parameters are passed by pointer, while they are actually passed by value Then on a deeper check of the C code, found it at https://github.com/ada-url/ada/blob/1227f60798b05a04412af867d2f13ed20ead9243/include/ada_c.h#L53C6-L53C27. BREAKING CHANGE: ada_free_owned_string parameter pass by value
Thanks for doing this! Is it easy to add a test for this? |
@steveklabnik can you take a look? :-) |
@anonrig I tried this approach I added this to ffi.rs #[cfg(test)]
mod tests {
use crate::ffi;
#[test]
fn ada_free_owned_string_works() {
let str = "meßagefactory.ca";
let result = unsafe {ffi::ada_idna_to_ascii(str.as_ptr().cast(), str.len())};
assert_eq!(result.as_ref(), "xn--meagefactory-m9a.ca");
unsafe {ffi::ada_free_owned_string(result)};
}
} The previous approach would cause a crash. I am not sure how correct this is tbh, because it is also dependent on ada_idna_to_ascii but at the end of the day, I think this is the cleanest approach. |
@pratikpc looks good to me. can you add it to the PR? |
Add a single test case for ada_free_owned_string This way, the provided function is always present and always used in the codebase. Test case is based on the previously present test case for idna_to_ascii
@anonrig done! |
Had not previously formatted the code
There were formatting issues Fixed Would it be possible to add formatting to the Justfile? Edit: Maybe a new MR for the same? |
Yup |
Seems like this got sorted, looks fine to me :) |
While working on #63, I noticed that my Drop wasn't correctly working because the Rust Implementation was assuming that the C parameters are passed by pointer, while they are actually passed by value
Then on a deeper check of the C code, found it at https://github.com/ada-url/ada/blob/1227f60798b05a04412af867d2f13ed20ead9243/include/ada_c.h#L53C6-L53C27.
BREAKING CHANGE: ada_free_owned_string parameter pass by value not pointer. Closes #64