-
Notifications
You must be signed in to change notification settings - Fork 63
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
Support Option<Vec<T>> where T is primitive #147
Conversation
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 really great work.
Left some minor feedback but overall this looks good.
Thanks for diving in and figuring all of this out.
vec.push(value: 123) | ||
vec.push(value: 321) | ||
let refrelct = rust_reflect_option_vector_rust_type(vec) | ||
XCTAssertEqual(vec.len(), 2) |
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 only reason that this works is because vec
doesn't get dropped when you call rust_reflect_option_vector_rust_type
.
Otherwise this would be a use after free.
You can change this to:
// ...
let reflected = rust_reflect_option_vector_rust_type(vec)
XCTAssertEqual(reflected.len(), 2)
XCTAssertEqual(reflected.get(index: 0), 123)
XCTAssertEqual(reflected.get(index: 1), 321)
// ...
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.
Oh and please also update the example in the PR body. I plan to copy and paste it into the release notes.
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.
I plan to copy and paste it into the release notes.
Do I also need to change rust-code?
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.
It's fine I'll rename it to something shorter in the release notes
@@ -94,6 +94,18 @@ class OptionTests: XCTestCase { | |||
XCTAssertNil(rust_reflect_option_str(none)) | |||
} | |||
|
|||
func testSwiftCallRustWithOptionVecType() throws { |
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.
testSwiftCallRustWithOptionVecOfPrimitiveType
@@ -4,6 +4,70 @@ use super::{CodegenTest, ExpectedCHeader, ExpectedRustTokens, ExpectedSwiftCode} | |||
use proc_macro2::TokenStream; | |||
use quote::quote; | |||
|
|||
/// Test code generation for Rust function that accepts and returns an Option<Vec<T>> where T is a |
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.
MINOR:
Can we move this test below extern_rust_fn_option_primitive
.
I think that the tests are easier to follow if we start with the simple scenarios are gradually build up to more complex scenarios.
@@ -90,7 +90,13 @@ impl BridgedOption { | |||
} |
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.
Awesome work in this file
@@ -4,6 +4,70 @@ use super::{CodegenTest, ExpectedCHeader, ExpectedRustTokens, ExpectedSwiftCode} | |||
use proc_macro2::TokenStream; | |||
use quote::quote; | |||
|
|||
/// Test code generation for Rust function that accepts and returns an Option<Vec<T>> where T is a | |||
/// primitive. | |||
mod exterun_rust_fn_option_vector_primitive { |
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.
MINOR:
typo in extern
); | ||
|
||
#[test] | ||
fn exterun_rust_fn_option_vector_primitive() { |
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.
MINOR:
typo in extern
Addressed your review! Please let me know if there are any omissions or errors. |
Looks great! |
This PR addresses #146 and introduces
Option<Vec<T>>
whereT
is primitive.Here's an example of using
Option<Vec<T>>
: