Optimize `RustStr`'s `Equatable` implementation #151

NiwakaDev · 2023-01-29T02:22:46Z

Addresses #148.

chinedufn

Minor notes then looks good to me.

Great work here, thanks for figuring this out!

chinedufn · 2023-01-29T02:25:53Z

SwiftRustIntegrationTestRunner/SwiftRustIntegrationTestRunnerTests/StringTests.swift

+        }
+        XCTContext.runActivity(named: "Should not be equal"){
+            _ in
+            //Equal length


minor: The Equal length and Not equal length are on the wrong tests. Need to switch them.

chinedufn · 2023-01-29T02:26:42Z

crates/swift-bridge-build/src/generate_core/rust_string.c.h

+struct RustStr __swift_bridge__$RustString$trim(void* self);
+bool __swift_bridge__equality_operator_for_RustStr(struct RustStr lhs, struct RustStr rhs);


__swift_bridge__$RustStr$partial_eq

chinedufn · 2023-01-29T02:27:07Z

src/lib.rs

@@ -92,5 +92,25 @@ pub extern "C" fn __swift_bridge__null_pointer() -> *const std::ffi::c_void {
    std::ptr::null()
 }

+#[no_mangle]
+#[doc(hidden)]
+pub extern "C" fn __swift_bridge__equality_operator_for_RustStr(


__swift_bridge__RustStr__partial_eq

chinedufn · 2023-01-29T02:38:54Z

src/lib.rs

+        return false;
+    }
+
+    unsafe {


Another alternative that I thought of was:

unsafe { std::slice::from_raw_parts(lhs.start, lhs.len) == std::slice::from_raw_parts(rhs.start, rhs.len) }

I was thinking that the stdlib might have an optimization that I was unfamiliar with, but I think that desugars to https://doc.rust-lang.org/1.52.1/src/core/slice/cmp.rs.html#69-75

Which looks like the same as what you're doing. So this looks good to me.

chinedufn · 2023-01-29T02:39:02Z

src/lib.rs

@@ -92,5 +92,25 @@ pub extern "C" fn __swift_bridge__null_pointer() -> *const std::ffi::c_void {
    std::ptr::null()
 }

+#[no_mangle]


You can put this in std_bridge/string.rs under here

swift-bridge/src/std_bridge/string.rs

Lines 61 to 82 in 23d60fe

impl RustStr {

pub fn len(&self) -> usize {

self.len

}

// TODO: Think through these lifetimes and the implications of them...

pub fn to_str<'a>(self) -> &'a str {

let bytes = unsafe { std::slice::from_raw_parts(self.start, self.len) };

std::str::from_utf8(bytes).expect("Failed to convert RustStr to &str")

}

pub fn to_string(self) -> String {

self.to_str().to_string()

}

pub fn from_str(str: &str) -> Self {

RustStr {

start: str.as_ptr(),

len: str.len(),

}

}

}

I got the link error like:

Testing failed: Undefined symbol: ___swift_bridge__$RustStr$partial_eq Testing cancelled because the build failed.

How does the Swift-compiler know where methods are, such as len, to_string.

swift-bridge/src/std_bridge/string.rs

Lines 62 to 80 in 0de0dc8

pub fn len(&self) -> usize {

self.len

}

// TODO: Think through these lifetimes and the implications of them...

pub fn to_str<'a>(self) -> &'a str {

let bytes = unsafe { std::slice::from_raw_parts(self.start, self.len) };

std::str::from_utf8(bytes).expect("Failed to convert RustStr to &str")

}

pub fn to_string(self) -> String {

self.to_str().to_string()

}

pub fn from_str(str: &str) -> Self {

RustStr {

start: str.as_ptr(),

len: str.len(),

}

Seaching for RustStr's methods with a tool like grep, I couldn't find any declarations of them in any c-header files.

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

Lines 16 to 38 in e8d3859

pub(super) fn write_core_swift_and_c(out_dir: &Path) {

let core_swift_out = out_dir.join("SwiftBridgeCore.swift");

let mut swift = core_swift();

swift += "\n";

swift += &RUST_STRING_SWIFT;

swift += "\n";

swift += &SWIFT_CALLBACK_SUPPORT_NO_ARGS_NO_RETURN;

swift += "\n";

swift += &SWIFT_RUST_RESULT;

std::fs::write(core_swift_out, swift).unwrap();

let core_c_header_out = out_dir.join("SwiftBridgeCore.h");

let mut c_header = core_c_header().to_string();

c_header += "\n";

c_header += &RUST_STRING_C;

c_header += "\n";

c_header += &C_CALLBACK_SUPPORT_NO_ARGS_NO_RETURN;

c_header += "\n";

c_header += &C_RESULT_SUPPORT;

std::fs::write(core_c_header_out, c_header).unwrap();

}

swift-bridge/crates/swift-bridge-build/src/generate_core/rust_string.c.h

Lines 17 to 18 in b654a24

struct RustStr __swift_bridge__$RustString$as_str(void* self);

struct RustStr __swift_bridge__$RustString$trim(void* self);

Oh wait, it looks like none of the RustStr methods like len or to_string are actually bridged. So that's why you couldn't find them.

But yeah you can declare the ___swift_bridge__$RustStr$partial_eq inside of rust_string.c.h

The build script might not be getting re-ran.

Try running touch crates/swift-integration-tests/build.rs or otherwise modifying and saving (then removing the modification of) the build.rs so that it gets re-run and seeing if that works.

https://doc.rust-lang.org/cargo/reference/build-scripts.html#life-cycle-of-a-build-script

Try running touch crates/swift-integration-tests/build.rs or otherwise modifying and saving

I got the link error again.

RustString's methods, such as len() and as_str(), seem to be imported on the Swift side.

However, RustStr's methods don't seem to be imported on the Swift side.

I tried to dive into this problem, and run objdump -d libswift_integration_tests.a | grep RustStr:

0000000000000328 <__ZN12swift_bridge10std_bridge6string7RustStr10partial_eq17h39b2f23e5457666eE

patial_eq might be not __swift_bridge__$RustStr$partial_eq but __ZN12swift_bridge10std_bridge6string7RustStr10partial_eq17h39b2f23e5457666eE.

I just fetched your branch and I can reproduce the error. Give me a sec

This should work:

// std_bridge/string.rs impl RustStr { // ... } impl PartialEq for RustStr { fn eq(&self, other: &Self) -> bool { unsafe { std::slice::from_raw_parts(self.start, self.len) == std::slice::from_raw_parts(other.start, other.len) } } } #[export_name = "__swift_bridge__$RustStr$partial_eq"] #[allow(non_snake_case)] pub extern "C" fn __swift_bridge__RustStr_partial_eq(lhs: RustStr, rhs: RustStr) -> bool { lhs == rhs }

I'm sorry for taking up your time. I'm going to study rust hard.

chinedufn · 2023-01-29T02:41:31Z

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

-        // on the Rust side that compares the underlying byte slices.
-        return
-            lhs.toString() == rhs.toString()
+        return __swift_bridge__equality_operator_for_RustStr(lhs, rhs);


chinedufn · 2023-01-29T11:35:21Z

src/lib.rs

+    lhs: string::RustStr,
+    rhs: string::RustStr,
+) -> bool {
+    if lhs.len != rhs.len {


Let's make this lhs == rhs

And then you can

impl PartialEq for RustStr { .. }

And handle the comparison inside of the PartialEq impl.

chinedufn · 2023-01-29T13:56:29Z

src/std_bridge/string.rs

@@ -79,4 +81,15 @@ impl RustStr {
            len: str.len(),
        }
    }
+
+    pub fn partial_eq(


Should move this into a PartialEq trait implementation.

impl PartialEq for RustStr { // ... }

https://doc.rust-lang.org/std/cmp/trait.PartialEq.html

Should move this into a PartialEq trait implementation.

Sorry...I know that I should do it, but I want to solve the link error first. So, I pushed 03245ec to share this problem.

NiwakaDev · 2023-01-29T17:50:25Z

I addressed your review. Sorry...

chinedufn · 2023-01-29T18:46:17Z

Great work!

NiwakaDev added 3 commits January 29, 2023 11:20

Fix RustStr's == operator calling toString

971b28c

Update src/lib.rs

0e3c738

Format src/lib.rs

df6a605

chinedufn reviewed Jan 29, 2023

View reviewed changes

chinedufn changed the title ~~Fix RustStr's == operator calling toString~~ Optimize RustStr's Equatable implementation Jan 29, 2023

chinedufn reviewed Jan 29, 2023

View reviewed changes

Add __swift_bridge__ to rust_string.c.h

03245ec

chinedufn reviewed Jan 29, 2023

View reviewed changes

Address review

f6a1806

chinedufn merged commit 5428646 into chinedufn:master Jan 29, 2023

chinedufn mentioned this pull request Jan 29, 2023

Optimize RustStr's Equatable implementation #148

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Optimize `RustStr`'s `Equatable` implementation #151

Optimize `RustStr`'s `Equatable` implementation #151

NiwakaDev commented Jan 29, 2023

chinedufn left a comment

chinedufn Jan 29, 2023

chinedufn Jan 29, 2023

chinedufn Jan 29, 2023

chinedufn Jan 29, 2023

chinedufn Jan 29, 2023 •

edited

Loading

NiwakaDev Jan 29, 2023

NiwakaDev Jan 29, 2023

chinedufn Jan 29, 2023

chinedufn Jan 29, 2023 •

edited

Loading

chinedufn Jan 29, 2023 •

edited

Loading

NiwakaDev Jan 29, 2023 •

edited

Loading

chinedufn Jan 29, 2023 •

edited

Loading

chinedufn Jan 29, 2023 •

edited

Loading

NiwakaDev Jan 29, 2023

chinedufn Jan 29, 2023

chinedufn Jan 29, 2023

chinedufn Jan 29, 2023

NiwakaDev Jan 29, 2023 •

edited

Loading

NiwakaDev commented Jan 29, 2023

chinedufn commented Jan 29, 2023

		struct RustStr __swift_bridge__$RustString$trim(void* self);
		bool __swift_bridge__equality_operator_for_RustStr(struct RustStr lhs, struct RustStr rhs);

	impl RustStr {
	pub fn len(&self) -> usize {
	self.len
	}

	// TODO: Think through these lifetimes and the implications of them...
	pub fn to_str<'a>(self) -> &'a str {
	let bytes = unsafe { std::slice::from_raw_parts(self.start, self.len) };
	std::str::from_utf8(bytes).expect("Failed to convert RustStr to &str")
	}

	pub fn to_string(self) -> String {
	self.to_str().to_string()
	}

	pub fn from_str(str: &str) -> Self {
	RustStr {
	start: str.as_ptr(),
	len: str.len(),
	}
	}
	}

	pub(super) fn write_core_swift_and_c(out_dir: &Path) {
	let core_swift_out = out_dir.join("SwiftBridgeCore.swift");
	let mut swift = core_swift();
	swift += "\n";
	swift += &RUST_STRING_SWIFT;
	swift += "\n";
	swift += &SWIFT_CALLBACK_SUPPORT_NO_ARGS_NO_RETURN;
	swift += "\n";
	swift += &SWIFT_RUST_RESULT;

	std::fs::write(core_swift_out, swift).unwrap();

	let core_c_header_out = out_dir.join("SwiftBridgeCore.h");
	let mut c_header = core_c_header().to_string();
	c_header += "\n";
	c_header += &RUST_STRING_C;
	c_header += "\n";
	c_header += &C_CALLBACK_SUPPORT_NO_ARGS_NO_RETURN;
	c_header += "\n";
	c_header += &C_RESULT_SUPPORT;

	std::fs::write(core_c_header_out, c_header).unwrap();
	}

Optimize RustStr's Equatable implementation #151

Optimize RustStr's Equatable implementation #151

Conversation

NiwakaDev commented Jan 29, 2023

chinedufn left a 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

Choose a reason for hiding this comment

chinedufn Jan 29, 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

chinedufn Jan 29, 2023 • edited Loading

Choose a reason for hiding this comment

chinedufn Jan 29, 2023 • edited Loading

Choose a reason for hiding this comment

NiwakaDev Jan 29, 2023 • edited Loading

Choose a reason for hiding this comment

chinedufn Jan 29, 2023 • edited Loading

Choose a reason for hiding this comment

chinedufn Jan 29, 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

NiwakaDev Jan 29, 2023 • edited Loading

Choose a reason for hiding this comment

NiwakaDev commented Jan 29, 2023

chinedufn commented Jan 29, 2023

Optimize `RustStr`'s `Equatable` implementation #151

Optimize `RustStr`'s `Equatable` implementation #151

chinedufn Jan 29, 2023 •

edited

Loading

chinedufn Jan 29, 2023 •

edited

Loading

chinedufn Jan 29, 2023 •

edited

Loading

NiwakaDev Jan 29, 2023 •

edited

Loading

chinedufn Jan 29, 2023 •

edited

Loading

chinedufn Jan 29, 2023 •

edited

Loading

NiwakaDev Jan 29, 2023 •

edited

Loading