Replace `ResultVoidAndPtr` with pointer #184

NiwakaDev · 2023-03-05T00:18:59Z

This PR replaces ResultVoidAndPtr with a pointer. Note that this PR doesn't affect the user experience at all.

related to #183.

For example, the following bridge module:

#[swift_bridge::bridge]
mod ffi {
    extern "Rust" {
        type SomeType;

        fn some_function () -> Result<(), SomeType>;
    }
}

Will now generate the following foreign function interface:

#[export_name = "__swift_bridge__$some_function"]
pub extern "C" fn __swift_bridge__some_function() -> *mut super::SomeType {
    match super::some_function() {
        Ok(ok) => std::ptr::null_mut(),
        Err(err) => Box::into_raw(Box::new({
            let val: super::SomeType = err;
            val
        })) as *mut super::SomeType
    }
}

chinedufn

Nice!!

Left some minor feedback then after that this looks good to me.

Thanks!

chinedufn · 2023-03-05T00:27:20Z

crates/swift-bridge-ir/src/bridged_type/bridgeable_result.rs

        if self.is_custom_result_type() {
            let ty = format_ident!("{}", self.c_struct_name());
            return quote! {
                #ty
            };
        }
+
+        if self.ok_ty.can_be_encoded_with_zero_bytes() {
+            let err_ty = self.err_ty.to_rust_type_path(types);


I think we want return self.err_ty.to_ffi_compatible_rust_type(swift_bridge_path, types);

chinedufn · 2023-03-05T00:29:37Z

crates/swift-bridge-ir/src/bridged_type/bridgeable_result.rs

+            .convert_ffi_expression_to_swift_type("val.ok_or_err!", type_pos, types);
+        let err =
+            self.err_ty
+                .convert_ffi_expression_to_swift_type("val.ok_or_err!", type_pos, types);

        // There is a Swift compiler bug in Xcode 13 where using an explicit `()` here somehow leads


Need to move the comment since we moved the code that it was on

chinedufn · 2023-03-05T00:32:45Z

crates/swift-bridge-ir/src/bridged_type/bridgeable_result.rs

@@ -430,15 +424,16 @@ impl BuiltInResult {
        if !self.is_custom_result_type() {


Can we delete this panic since we already have extensive testing for this case

Can rename the method from c_struct_name to custom_c_struct_name to make it more clear that this is only used for custom results.

chinedufn · 2023-03-05T00:33:09Z

crates/swift-bridge-ir/src/bridged_type/bridgeable_result.rs

@@ -430,15 +424,16 @@ impl BuiltInResult {
        if !self.is_custom_result_type() {
            panic!("Should not be called when this type is not a custom result type.")
        }
+
        let ok = &self.ok_ty;
        let err = &self.err_ty;

        if ok.can_be_encoded_with_zero_bytes() && err.is_passed_via_pointer() {


Can we delete this panic since we already have extensive testing for this case

chinedufn · 2023-03-05T00:33:12Z

crates/swift-bridge-ir/src/bridged_type/bridgeable_result.rs

        let ok = &self.ok_ty;
        let err = &self.err_ty;

        if ok.can_be_encoded_with_zero_bytes() && err.is_passed_via_pointer() {
-            return "ResultVoidAndPtr".to_string();
+            panic!("Should not be called when this type is a ResultVoidAndPtr")
        }

        if ok.is_passed_via_pointer() && err.can_be_encoded_with_zero_bytes() {


Can we delete this panic since we already have extensive testing for this case

chinedufn · 2023-03-05T00:36:27Z

src/std_bridge/result.rs

@@ -44,12 +44,3 @@ pub struct ResultPtrAndPtr {
    pub is_ok: bool,
    pub ok_or_err: *mut std::ffi::c_void,
 }
-


Need to also delete the C support

swift-bridge/crates/swift-bridge-build/src/generate_core/result_support.rs

Lines 42 to 44 in 27d7be9

pub const C_RESULT_VOID_SUPPORT: &'static str = r#"

struct __private__ResultVoidAndPtr { bool is_ok; void* err; };

"#;

swift-bridge/crates/swift-bridge-build/src/generate_core.rs

Lines 38 to 39 in 27d7be9

c_header += "\n";

c_header += &C_RESULT_VOID_SUPPORT;

chinedufn · 2023-03-05T00:43:32Z

Mind updating the PR body with a visual indication of what this does?

For example, a simple bridge module and the corresponding generated Rust extern "C" fn.

Here's an example (you don't need to do a write up like that PR has, just some quick example code is fine) #178

Reasons:

Your example code will get included in the commit message when I rebase the PR, so the commit message will be of higher quality
If someone is ever implementing a similar optimization we can link them to this PR and they can quickly get a sense of what it's doing

In general, if a PR touches our code generation then the PR body should visually illustrate the change. Doesn't need to be anything long or fancy. Just a simple bridge module and any relevant generated code is fine.

NiwakaDev · 2023-03-05T01:32:49Z

Addressed your review. Please let me know if any omissions or errors.

chinedufn · 2023-03-05T01:42:52Z

crates/swift-bridge-ir/src/bridged_type/bridgeable_result.rs

@@ -232,6 +234,20 @@ impl BuiltInResult {
                TypePosition::SwiftCallsRustAsyncOnCompleteReturnTy => todo!(),
            };
        }
+
+        // There is a Swift compiler bug in Xcode 13 where using an explicit `()` here somehow leads


This needs to go above if self.ok_ty.is_null() like it was before

chinedufn · 2023-03-05T01:43:08Z

crates/swift-bridge-ir/src/bridged_type/bridgeable_result.rs

-            let err_ty = self.err_ty.to_rust_type_path(types);
+            let err_ty = self
+                .err_ty
+                .to_ffi_compatible_rust_type(swift_bridge_path, types);
            return quote! {


No need to quote!

chinedufn · 2023-03-05T01:43:32Z

crates/swift-bridge-build/src/generate_core.rs

@@ -36,7 +34,6 @@ pub(super) fn write_core_swift_and_c(out_dir: &Path) {
    c_header += "\n";
    c_header += &C_RESULT_SUPPORT;
    c_header += "\n";


Can remove this new line

NiwakaDev · 2023-03-05T01:58:59Z

Done. But, there might be any omissions or errors.

chinedufn · 2023-03-05T02:17:06Z

Thanks!

NiwakaDev added 2 commits March 5, 2023 09:15

replace ResultVoidAndPtr with pointer

7e6e9ee

cargo fmt

4a6a2a2

chinedufn reviewed Mar 5, 2023

View reviewed changes

NiwakaDev added 6 commits March 5, 2023 09:57

delete panic

e812de5

rename c_struct_name custom_c_struct_name

f71af8c

delete C_RESULT_VOID_SUPPORT

bb8e2e4

replace to_rust_type_path with to_ffi_compatible_rust_type

4c863b3

move comment

348b31c

cargo fmt

8347839

NiwakaDev changed the title ~~Replace ResultVoidAndPtr with pointer~~ Replace ResultVoidAndPtr with pointer Mar 5, 2023

chinedufn reviewed Mar 5, 2023

View reviewed changes

address review

4fd7019

chinedufn merged commit 20baa73 into chinedufn:master Mar 5, 2023

chinedufn mentioned this pull request Mar 5, 2023

Replace ResultVoidAndPtr with a pointer (null pointer optimization) #183

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Replace `ResultVoidAndPtr` with pointer #184

Replace `ResultVoidAndPtr` with pointer #184

NiwakaDev commented Mar 5, 2023 •

edited

Loading

chinedufn left a comment

chinedufn Mar 5, 2023 •

edited

Loading

chinedufn Mar 5, 2023

chinedufn Mar 5, 2023

chinedufn Mar 5, 2023 •

edited

Loading

chinedufn Mar 5, 2023

chinedufn Mar 5, 2023

chinedufn Mar 5, 2023

chinedufn Mar 5, 2023

chinedufn commented Mar 5, 2023 •

edited

Loading

NiwakaDev commented Mar 5, 2023

chinedufn Mar 5, 2023

chinedufn Mar 5, 2023

chinedufn Mar 5, 2023

NiwakaDev commented Mar 5, 2023

chinedufn commented Mar 5, 2023

		@@ -430,15 +424,16 @@ impl BuiltInResult {
		if !self.is_custom_result_type() {

	pub const C_RESULT_VOID_SUPPORT: &'static str = r#"
	struct __private__ResultVoidAndPtr { bool is_ok; void* err; };
	"#;

Replace ResultVoidAndPtr with pointer #184

Replace ResultVoidAndPtr with pointer #184

Conversation

NiwakaDev commented Mar 5, 2023 • edited Loading

chinedufn left a comment

Choose a reason for hiding this comment

chinedufn Mar 5, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chinedufn Mar 5, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chinedufn commented Mar 5, 2023 • edited Loading

NiwakaDev commented Mar 5, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NiwakaDev commented Mar 5, 2023

chinedufn commented Mar 5, 2023

Replace `ResultVoidAndPtr` with pointer #184

Replace `ResultVoidAndPtr` with pointer #184

NiwakaDev commented Mar 5, 2023 •

edited

Loading

chinedufn Mar 5, 2023 •

edited

Loading

chinedufn Mar 5, 2023 •

edited

Loading

chinedufn commented Mar 5, 2023 •

edited

Loading