Skip to content
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

Merged
merged 3 commits into from
May 20, 2024
Merged

fix(ffi/ada_free_owned_string): c parameter is passed by value #65

merged 3 commits into from
May 20, 2024

Conversation

pratikpc
Copy link
Contributor

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

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
@anonrig
Copy link
Member

anonrig commented May 20, 2024

Thanks for doing this! Is it easy to add a test for this?

@anonrig
Copy link
Member

anonrig commented May 20, 2024

@steveklabnik can you take a look? :-)

@pratikpc
Copy link
Contributor Author

pratikpc commented May 20, 2024

@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.

@anonrig
Copy link
Member

anonrig commented May 20, 2024

@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
@pratikpc
Copy link
Contributor Author

@anonrig done!

Had not previously formatted the code
@pratikpc
Copy link
Contributor Author

pratikpc commented May 20, 2024

There were formatting issues

Fixed

Would it be possible to add formatting to the Justfile?

Edit:

Maybe a new MR for the same?

@anonrig
Copy link
Member

anonrig commented May 20, 2024

Would it be possible to add formatting to the Justfile?

Yup

@anonrig anonrig merged commit b1633d7 into ada-url:main May 20, 2024
6 checks passed
@pratikpc pratikpc deleted the patch-1 branch May 20, 2024 13:46
@steveklabnik
Copy link
Contributor

Seems like this got sorted, looks fine to me :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ada_free_owned_string parameter should be passed by value
3 participants