-
Notifications
You must be signed in to change notification settings - Fork 65
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 returning Result<Vec<>, ...> from Rust #218
Conversation
Thank you for your contribution!! I`m not the owner of this repository, but I left one minor feedback. We need to add a codegen test because we'd like to test that |
@@ -481,6 +481,7 @@ impl BridgeableType for BridgedType { | |||
|
|||
fn is_passed_via_pointer(&self) -> bool { | |||
match self { | |||
BridgedType::StdLib(StdLibType::Vec(_)) => true, |
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.
If we use Result<Vec<RustOpaqueType>, RustOpaqueType>
, we need to test that __private__ResultPtrAndPtr
is generated, not a custom result type.
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.
As mentioned in my review I think this wouldn't need a codegen test but maybe instead we'd have some tests in the BuiltInResult
file where we check which FFI representation is used by different Result<T, E>
signatures.
So, we'd essentially be testing this function
fn is_custom_result_type(&self) -> bool { |
Doesn't need to happen in this PR though.
Thanks a lot for reviewing this PR, very helpful!!! Thank you!
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.
@NiwakaDev thanks a lot for reviewing! I'd love if you continued to help with reviewing PRs since at this point you have a LOT of experience with the internals.
For this particular PR, I don't think that we need a codegen test since we already have a bunch of codegen tests for the different flavors of Result<T, E>
.
If we wanted to test whether or not the PtrAndPtr
was used I think we'd just add a test where we parse different Result<T, E>
signatures into BuiltInResult
and assert whether or not a custom FFI representation is used.. Or something like that..
Don't think we need a codegen test for every possible Result signature, just the different "flavors" of Result.
So, I think that this PR is fine with just the integration test and can land.
Just lmk if you disagree.
@jfaust can you please update the PR body with an example bridge module illustrating what this PR enables (so, just a bridge module showing a function that returns a Result<Vec<u32>, ()>
or something.
We like to have every PR show exactly what has been made possible.
Useful when we link new contributors to old PRs, as well as when we create release notes.
After that we can merge this.
Thanks!
Sure! |
@chinedufn done! |
d83eb75
to
c5d2e28
Compare
Thanks! |
Fixes #217
This supports bridges of the form: